-
Notifications
You must be signed in to change notification settings - Fork 1.4k
add {PROVIDER}_DISPLAY_NAME env-var #8967
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
add {PROVIDER}_DISPLAY_NAME env-var #8967
Conversation
3687036
to
dbcd12d
Compare
Connected to Huly®: UBERF-10629 |
1 similar comment
Connected to Huly®: UBERF-10629 |
510a8f6
to
112a908
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. Could you please also attach a screenshot of how it looks when configured with a custom name.
@@ -2,9 +2,12 @@ | |||
import { Label } from '@hcengineering/ui' | |||
import OpenId from '../icons/OpenId.svelte' | |||
import login from '../../plugin' | |||
import { getMetadata } from '@hcengineering/platform' | |||
|
|||
const openIdDisplayName = getMetadata(login.metadata.DisableSignUp) ?? 'OpenId' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong metadata being read?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Must have hit Ctrl-Z once too much there 😄
server/front/src/starter.ts
Outdated
@@ -119,6 +119,8 @@ export function startFront (ctx: MeasureContext, extraConfig?: Record<string, st | |||
|
|||
const disableSignUp = process.env.DISABLE_SIGNUP | |||
|
|||
const openIdDisplayName = process.env.OPENID_DISPLAY_NAME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to define this variable in the accounts service along with all other variables defining the provider and return it along with the providers' ids in the /providers
request? It'd help to simplify things:
- The whole provider entity is defined in one place
- Handled with one piece of code for both the web ui and the desktop
- Will also be available to any other clients besides the web ui and the desktop (e.g. automation, custom integrations, etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I still committed a fix for the current approach for now and will have a look into this next week
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so I made a new version including it in the /providers request. I added the functionality to GitHub and google providers too for the sake of consistency (GOOGLE_DISPLAY_NAME, GITHUB_DISPLAY_NAME). Even though it probably won't really be used there.
36b8cf2
to
7a6525f
Compare
…on (configurable via *_PROVIDER_NAME) Signed-off-by: nicolasschneider <[email protected]>
7a6525f
to
c313db2
Compare
This environment variable is added to the front-service to allow setting a custom provider name for OpenId Login button: