-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: implement theme changing feature to propagate changes to child apps #48
Conversation
c3e525c
to
499c44a
Compare
Thank you for your feedback! @wwills2 I have implemented a new flow for changing colors using theme.js, so the color-changing component is no longer necessary. Additionally, I have used arrow functions for consistency as told. I appreciate you pointing this out. Please let me know if there are any other concerns or suggestions. |
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.
hi @newweborder007, there are 3 changes that need to be made
- please remove the /public folder and its contents from version control. they do not need to be part of the code base
- place the custom theme in a separate state, and do not send a theme or colors message to the iframes if theme.json was not able to be fetched.
- the header branding SVG functionality is still missing. the header branding SVG should be placed on the header and centered in the middle of the header.
3B. for HeaderBranding.svg, please use the same pattern that you used for CadtLogo.jsx, ExplorerLogo.jsx, and TokenizationLogo,jsx. For HeaderBranding.jsx, if the component fails to fetch HeaderBranding.svg, then it should return an empty fragment.
for #3 here is a representative mockup of the size and location of HeaderBranding.svg.
const themeResponse = await fetch("/theme.json"); | ||
if (themeResponse.ok) { | ||
const customTheme = await themeResponse.json(); | ||
setTheme(customTheme); |
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.
please set the custom theme in a separate state. the theme should not be sent to the child UI's unless it is a custom theme loaded from theme.json
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.
have look at line 92, theme wont be sent to the child ui's unless there's a loaded theme from theme.json.
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.
my apologies, i misread that. thank you for the clarification
Hello @wwills2, Changes have been implemented and pushed.
Note: Couldn't remove /public folder because it have files that are already being used by project. Additionally, I just put the custom theme and svgs for structure and names reference in future. Files I created aren't effecting anything. theme.json have default colors, and svgs are same as they were. If you delete the files I created, It wont effect anything. |
The /public folder directly places files in the server root, which means the only way to test the implemented feature was by placing the necessary files in this directory. |
this is true, but when the software is deployed if custom icons and/or colors are desired, the deploying entity will provide them. we do not want the files to ship with the software. they do not need to be deleted locally, they just need to be un-versioned by git so that they are not part of the code base on github. |
This PR introduces a theme-changing feature in the parent application, enabling theme changes to be propagated and reflected across all child apps.
Changes