-
Notifications
You must be signed in to change notification settings - Fork 619
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
New Admin UI - Colors and Theming #4474
New Admin UI - Colors and Theming #4474
Conversation
…o feat/theming-token-improvements
…into feat/theming-token-improvements # Conflicts: # apps/admin/package.json # package.json # yarn.lock
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.
Well done, Adrian! 🎉
This solution is far better than I imagined. I left a couple of minor comments...but the PR is ready to be merged. Thank you 🙏
try { | ||
const localData = localStorage.getItem(LOCAL_STORAGE_KEY); | ||
if (localData) { | ||
return JSON.parse(localData); | ||
} | ||
} catch {} |
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.
Do we need the try/catch
syntax here?
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.
It's there just because getItem
might not return string JSON, so this way we catch that case, without even checking the value of getItem
first.
@@ -24,7 +24,7 @@ export type TopAppBarProps = RmwcTopAppBarProps & { | |||
const TopAppBar = (props: TopAppBarProps) => { | |||
const { children, ...rest } = props; | |||
return ( | |||
<RmwcTopAppBar {...rest}> | |||
<RmwcTopAppBar {...rest} className={"bg-primary-default"}> |
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.
When we are going to support the Tailwind prefix wby-
, this class name will not work as expected.
This is just a reminder we need to be aware of. 😅
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.
Yes, good point. 🙏🏻
…into feat/theming-token-improvements # Conflicts: # packages/admin-ui/scripts/importFromFigma/exports/Alias tokens.json
Changes
For starters, this PR brings in the primitive tokens for colors (check theme out here).
With these tokens now in
theme.scss
, we now reference them in alias tokens. Previously, alias tokens directly had color codes assigned to them.All of this ultimately enables easier Admin theme customization, which is done via the new
AdminConfig.Theme
React component. Here are a couple of examples.Example 1 - Basic Theme Assignment
Basic theme assignment is pretty simple when it comes to code.
Instead of just defining a single color, users can also define all of the shades in a palette:
Example 2 - Dynamic Theme Assignment
In this example, we're assigning a theme based on current tenant. Note the usage of
Plugin
component. This is required because otherwise, we can't read the current tenant. This is the only way to ensure that this code is executed once the tenant information has been loaded.Notes
There are a couple of things to note though.
1.
AdminConfig.Theme
Is a Regular React ComponentNote that the
AdminConfig.Theme
component is actually NOT a React properties component.This is something I discussed with Pavel, and we're basically doing it in order to speed up code execution, trying to get the theme applied as fast as possible (immediately upon its rendering).
We started discussing this when observing the visual issues during Admin app's initialization phase (described below).
2. Admin App / Initialization Phase - Loading / Visual Issues
Note
Note that all of the below is only relevant for dynamic themes, which is example 2 from the above code snippets. With basic theme assignment (example 1), everything looks/feels fine.
While testing the above code examples, I spotted an issue where the theme would not get applied on time, which would cause the user to see the default Webiny theme for a brief amount of time. More specifically: during the Admin app's init phase, user would see the loading spinner component in the default orange color, instead of the color provided via custom theme.
Why is this happening? Well, it's simply because of the way components are structured and because of the Admin app's init logic that's currently in place.
The basic example is
WcpProvider
that's:Because of 3 and 4, the rest of the Admin app init process is on hold, and this is the first reason why the theme cannot be yet applied and the user is seeing orange loading spinner.
For now, I've just removed the loading spinner here, and also did a bit of data caching, just so on subsequent requests this component immediately renders its children / doesn't block the rest of the Admin app init process.
But as I said, this is the basic example. There are more points to inspect here, for example:
Of course, I did try to play with these, and even got so far that I've removed all of these initial loading spinners, just so they're not rendered with the default theme color. But you know what.... ultimately, apart from WCP provider, I decided not to do anything, as this is a separate topic that needs further investigation. It's not related to Admin app UI renovations that we're doing here.
All in all, we'll need to revisit this at some point, but now is not the time.
And it's not like this wasn't already the case. Current theming system suffers from same issue, so we're not introducing a regression.
Additional Changes
1. Minor Cleanups
1.1.
packages/app-admin-cognito/src/components/StyledComponents.ts
Saw links in the sign in screen still orange, because there was some custom styling done with the
--mdc-theme-...
CSS variables.1.2.
packages/app-admin-rmwc/src/modules/Navigation/Styled.ts
Removed custom styling since it was no longer needed. There will be probably more to remove but for now didn't go too far.
1.3.
packages/app-admin/src/styles/material-theme-assignments.scss
Accidentally saw this file and decided to remove CSS that I was sure we won't be needing any more (old components). More cleaning up in this file will be needed.
2. Updated
app-serverless-cms
ExportsThis package now exports not only the new
AdminConfig
, but also the tenancy-related components. Better to have everything in one place (easier for users).3. Removed Component Mount Tracking
Component mount tracking was removed in
packages/app-tenant-manager/src/index.tsx
. This basically resolved an issue where, in a MT setup, in a specific case you would not see the tenant selector in top-right corner of the screen. Thanks goes to Pavel for this one.How Has This Been Tested?
Manually.
Documentation
Add theming doc later (noted here).