Update components to new DS theme#207
Conversation
VictoriaBeilsten-Edmands
requested review from
akademy
and removed request for
akademy
VictoriaBeilsten-Edmands
force-pushed
the
vbe/ds-theme-components
branch
3 times, most recently
from
e4957ee to
b375210
Compare
VictoriaBeilsten-Edmands
force-pushed
the
vbe/ds-theme-components
branch
from
b375210 to
b2a8d06
Compare
akademy
left a comment
akademy
left a comment
There was a problem hiding this comment.
I've added a few things. Also got the following questions (not about specific lines/files)
- AppTitleBar, no longer matches the NavBar colours. Is there some way to set it to the correct colours and add it as a Storybook example?
- Footer seems to have an odd background colour now. (You can see original matched the NavBar too https://diamondlightsource.github.io/sci-react-ui/?path=/docs/components-navigation-footer--docs )
- The ColourSchemeButton no longer looks cool. All the hover colours have gone. This is a sad day.
- The ColourSchemeButton also no longer matches the styling of the User control (i.e. with a filled background)
There was a problem hiding this comment.
What is the significance of the name change?
There was a problem hiding this comment.
To make it less confusing what "dark" means - is it dark text in the logo, or dark theme? Maybe a better name would be logo-on-dark-surface and logo-on-light-surface
| onClick={(event) => { | ||
| setColourScheme(isDark() ? ColourSchemes.Light : ColourSchemes.Dark); | ||
| if (props.onClick) props.onClick(event); | ||
| setMode?.(isDark ? "light" : "dark"); |
There was a problem hiding this comment.
When is setMode not defined? Also is mode also undefined? What happens then?
There was a problem hiding this comment.
Before the colour scheme context if fully initialized.
https://mui.com/material-ui/customization/css-theme-variables/configuration/
| style={style} | ||
| /> | ||
| ) | ||
| <img |
There was a problem hiding this comment.
Why is this nolonger using ImageColourSchemeSwitch ?
There was a problem hiding this comment.
I was dealing with the issue of brand-coloured background which is dark in both light/dark mode. I should be able to update ImageColourSchemeSwitch to deal with this though.
There was a problem hiding this comment.
This is using ImageColourSchemeSwitch again.
| }, | ||
| }; | ||
|
|
||
| export const DifferentBackgroundColour: Story = { |
There was a problem hiding this comment.
Are we pretending you can't change the background colour or can you actually not do this now?
There was a problem hiding this comment.
Just pretending
Co-authored-by: Matthew Wilcoxson <akademy@users.noreply.github.com>
VictoriaBeilsten-Edmands
force-pushed
the
vbe/ds-theme-components
branch
from
741dcac to
7d9db70
Compare
VictoriaBeilsten-Edmands
force-pushed
the
vbe/ds-theme-components
branch
from
1e1866f to
3e3611f
Compare
|
AppTitleBar, Footer, Breadcrumbs and Navbar all use the Bar component and can use all the colours (variant + surface combinations) that are available in Bar. They have their own defaults. I've added some stories for these. The tests have been updated to pass, but not updated to test any new logic. |
3 participants
No description provided.