-
Notifications
You must be signed in to change notification settings - Fork 351
Add branding customization support for logo and title #1339
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: main
Are you sure you want to change the base?
Conversation
| <img src={customLogoUrl} alt={title || 'Logo'} className="mr-5 size-20" /> | ||
| ) : ( | ||
| <AwsIcon className="mr-5 size-20" /> | ||
| ); |
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.
もう少しシンプルかつメモ化して実装したほうが良さそうな気がしました。
customLogoUrl を state で管理する必要がないと思うので、以下のような感じ。
const displayLogo = useMemo(() => {
// logoPath があったら <img/> を返し、なければ <AwsIcon/> を返す
}, [logoPath]);
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.
修正します
| const logoPath = import.meta.env.VITE_APP_BRANDING_LOGO_PATH; | ||
| const title = import.meta.env.VITE_APP_BRANDING_TITLE; | ||
|
|
||
| console.log('Branding config:', { logoPath, title }); |
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.
消し忘れ?
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.
おっと、、、消し忘れです
| const { enabled } = useUseCases(); | ||
| const { setIsShow, init } = useInterUseCases(); | ||
| const { t } = useTranslation(); | ||
| const { logoPath, title } = useBranding(); |
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.
これビルド時に組み込まれている環境変数から読み込んでいるだけの定数なので、hook 化するか微妙ですね。ragEnabled などと同様に、上部で定数として読み込んでも良い気がしました。なにか今後の展望的に hook 化したほうが良さそう的なことありますか?
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.
ありがとうございます。
無理やり理由つけるなら、将来的な話だと動的に変えたい、ってくらいですが、今そんな話は特にないので、環境変数を直接読むようにします。
Description of Changes
Add support for customizing the logo and title displayed on the landing page through a configuration file.
Checklist
npm run cdk:testand if there are snapshot differences, executenpm run cdk:test:update-snapshotto update snapshotsRelated Issues
#1338