Skip to content

Conversation

@jzempel
Copy link
Member

@jzempel jzempel commented Jul 9, 2024

Description

Completed recolor work for Chrome and Sheet components/subcomponents. πŸŽ—οΈ Set the storybook Nav control to false in order to visually test <Header isStandalone>.

Checklist

  • πŸ‘Œ design updates will be Garden Designer approved (add the designer as a reviewer)
  • 🌐 demo is up-to-date (npm start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 🀘 renders as expected with Bedrock CSS (?bedrock)
  • πŸ’‚β€β™‚οΈ includes new unit tests. Maintain existing coverage (always >= 96%)
  • β™Ώ tested for WCAG 2.1 AA accessibility compliance
  • πŸ“ tested in Chrome, Firefox, Safari, and Edge

Comment on lines +25 to +30
<ThemeProvider
theme={parentTheme => ({
...parentTheme,
colors: { ...parentTheme.colors, base: isLight ? 'light' : 'dark' }
})}
>
Copy link
Member Author

@jzempel jzempel Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coerce dark vs. light mode for Nav components rather than ferrying isLight & isDark props through to the various styled components.


const borderStyle = ({
const colorStyles = ({ theme, isOpen }: IStyledSheetProps & ThemeProps<DefaultTheme>) => {
const backgroundColor = getColor({ theme, variable: 'background.raised' });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would say Sheet's background would be default, as it is supplementary view alongside main content.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@steue yeah, agreed. Do we have corresponding designs for the Sheet component? Also, are the subtle borders still correct with the default background?

Copy link

@steue steue Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jzempel yeah! @'d you on Figma! πŸ’ͺ🏻 good catch on the border treatment. like modals and pane, surrounding border is default, with the header/footer using subtle.

Copy link

@steue steue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made a comment/fix on sheet's bg treatment. aside from that everything looks great! ty!


const isLight = hue ? isLightMemoized : false;
const isDark = hue ? !isLightMemoized : false;
const isLight = hue ? isLightMemoized : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To bette align with the IChromeContext type.

Suggested change
const isLight = hue ? isLightMemoized : undefined;
const isLight = hue ? isLightMemoized : false;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ze-flo the isLight is either true (custom light nav hue), false (custom dark nav hue), or undefined (default chromeHue) – in which case the data-test-light attribute should not be applied.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying. πŸ‘


interface IChromeContext {
hue: string;
isLight?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this key is always defined now.

Suggested change
isLight?: boolean;
isLight: boolean;

Comment on lines +12 to +25
export const getFooterHeight = (theme: DefaultTheme) => `${theme.space.base * 20}px`;

export const getHeaderHeight = (theme: DefaultTheme) => `${theme.space.base * 13}px`;

export const getHeaderItemSize = (theme: DefaultTheme) => `${theme.space.base * 7.5}px`;

export const getNavItemHeight = (theme: DefaultTheme) => getHeaderHeight(theme);

export const getNavWidth = (theme: DefaultTheme) => `${theme.space.base * 15}px`;

export const getNavWidthExpanded = () => `200px`;

export const getProductColor = (product?: Product, fallback = 'inherit') =>
product ? PALETTE.product[product] || fallback : fallback;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❀️

Copy link
Contributor

@ze-flo ze-flo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! πŸ”₯

}

return null;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ”₯ love a good reduction

@jzempel jzempel merged commit f1ee833 into next Jul 10, 2024
@jzempel jzempel deleted the jzempel/recolor-chrome branch July 10, 2024 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

6 participants