feat(react): adding ability to support theming of datahub, with two themes included OOB #2193
Conversation
|
@gabe-lyons : looks like there is a build issue |
| @@ -43,6 +47,14 @@ const client = new ApolloClient({ | |||
| }); | |||
|
|
|||
| const App: React.VFC = () => { | |||
| const [dynamicThemeConfig, setDynamicThemeConfig] = useState<Theme | null>(null); | |||
There was a problem hiding this comment.
Why not just set this to default?
There was a problem hiding this comment.
I see - is the idea that you can specify a "partial" theme that just overrides the default? This makes sense
| const themeConfig = dynamicThemeConfig || defaultThemeConfig; | ||
| Object.assign(overridesWithoutPrefix, themeConfig.styles); | ||
| Object.keys(overridesWithoutPrefix).forEach((key) => { | ||
| overridesWithoutPrefix[key.substring(1)] = overridesWithoutPrefix[key]; |
There was a problem hiding this comment.
Why do we remove the "@" here... what if people don't use the @ when they specify styles?
| )} | ||
| </EntityRegistryContext.Provider> | ||
| </Router> | ||
| <ThemeProvider theme={theme}> |
| @@ -38,7 +38,7 @@ export const BrowseResults = ({ | |||
| const entityRegistry = useEntityRegistry(); | |||
| return ( | |||
| <div> | |||
| <Content style={{ backgroundColor: 'white', padding: '25px 100px' }}> | |||
| <Content style={{ padding: '25px 100px' }}> | |||
There was a problem hiding this comment.
Should we extract inline styling?
| padding: 40px 100px; | ||
| `; | ||
|
|
||
| const BodyContainer = styled.div` |
| @@ -0,0 +1,32 @@ | |||
| { | |||
| "styles": { | |||
| "@border-radius-base": "4px", | |||
There was a problem hiding this comment.
If someone overrides as "border-radius-base", will this still work?
I still feel the @ convention is a bit confusing
| "assets": { | ||
| "logoUrl": "/assets/logo.png" | ||
| }, | ||
| "content": { |
There was a problem hiding this comment.
Really like that everything is in one place.
| @@ -0,0 +1,32 @@ | |||
| export type Theme = { | |||
| styles: { | |||
| 'border-radius-base': string; | |||
There was a problem hiding this comment.
Why don't these require the @ guy?
Light theme:
Dark theme:
Instructions on how to use in the readme
Checklist