Add BackgroundContext for gray background component theme changes #2364

benlister-okta · 2024-09-20T04:32:46Z

Summary

Adds BackgroundContext react context which can be used to display optimized contrast versions of certain components/component states
Background color detection is handled by BackgroundProvider automatically but can be overridden by adding contrastMode highContrast | lowContrast
Background context behavior is baked into OdysseyProvider, but is in its own file so it could be used outside of OdysseyProvider
Currently configured for isDisabled secondary Button, Status, and Tag
Uses Type augmentation to add the BackgroundProvider logic globally, requiring no component level changes for non-styled components.

Example usage:

<BackgroundProvider contrastMode="lowContrast"> <Button isDisabled label="Secondary" onClick={() => {}} variant="secondary" /> </BackgroundProvider>

<BackgroundProvider> <Button isDisabled label="Secondary" onClick={() => {}} variant="secondary" /> </BackgroundProvider>

Testing & Screenshots

I have confirmed this change with my designer and the Odyssey Design Team.

benlister-okta · 2024-09-20T19:25:33Z

packages/odyssey-storybook/src/components/odyssey-mui/Tooltip/Tooltip.stories.tsx

@@ -175,18 +175,6 @@ export const StatusWrapper: StoryObj<TooltipProps> = {
  },
 };

-export const Disabled: StoryObj<TooltipProps> = {


Removing this since the story doesn't function (tooltip on the disabled button) and it would require additional documentation for the background provider work.

Thanks. I removed this in my branch as well haha

benlister-okta · 2024-09-20T19:26:12Z

packages/odyssey-react-mui/src/theme/components.tsx

@@ -253,6 +253,8 @@ export const components = ({
            justifyContent: "center",
            alignItems: "center",
            borderRadius: 0,
+            borderWidth: 0,
+            borderBottomWidth: "1px",


Added this banner style change since its related to the contrast work and visual alignment with Figma (as of last week).

@benlister-okta The banner only has a border on the bottom?

That's correct. Alex and I discussed and went this direction since in the new template the guidance is (or will be) to use banner at the top of the browser. And for existing instances, we are ok with the look since it adds subtle contrast but still fits the system.

Is rendering this banner at the top of the browser/above the top nav even possible?

What I mean is, I thought the nav components will be injected into their apps. They may not have the ability to render anything at a higher level than the top nav

That makes sense, I am not sure how it will be implemented globally, it may need to be done at a feature level to support that. However, we tested it with the bottom-only border in the app and were fine with the look so it can be something we work towards in the future.

We may need to run this up the chain to determine if it is a breaking visual change or not. I know it's pretty minor but not sure how strict we need to be with stuff like this

I think it definitely warrants a message along with the release at very least. The border version was added 2 releases ago so its still a "new" change but we should make sure were letting people know.

KevinGhadyani-Okta · 2024-09-23T22:04:03Z

packages/odyssey-react-mui/src/OdysseyProvider.tsx

+}: OdysseyProviderProps<SupportedLanguages>) => {
+  const { isLowContrast } = useBackground();
+
+  const odysseyTokens = useMemo(
+    () => ({ ...Tokens, ...designTokensOverride }),
+    [designTokensOverride],
+  );
+


Instead of having OdysseyProvider consume BackgroundProvider, it should be baked in.

Also, we need a hook to calculate the background color of the parent element of OdysseyProvider. It needs to output a div for that to work.

We'd get the ref of the div and check against the DOM to compute the background-color of it.

Once we have that, the rest can be changed by having Surface and other "high contrast" components consume the BackgroundProvider and change the value.

bryancunningham-okta · 2024-09-25T18:04:14Z

packages/odyssey-react-mui/src/BackgroundContext.tsx

+  contrastMode?: ContrastMode;
+};
+
+export const BackgroundProvider = ({


@benlister-okta Can we name this something more descriptive. Maybe BackgroundContrastModeProvider? Or just ContrastModeProvider?

Call call, updated it to ContrastModeProvider

bryancunningham-okta · 2024-09-25T18:05:56Z

packages/odyssey-react-mui/src/BackgroundContext.tsx

+    () =>
+      createTheme({
+        ...existingTheme,
+        custom: {


Do we want to name this key something more descriptive than just custom?

We probably don't even need this top level object TBH. Any thoughts on just moving the contrast mode value to this level?

I improved this and moved the contrast mode into the augmentation but I don't believe we can get rid of this entirely since it's used to pass the variables into the theme in OdysseyProvider so it can be used globally.

bryancunningham-okta · 2024-09-25T18:07:38Z

packages/odyssey-react-mui/src/BackgroundContext.tsx

+        ...existingTheme,
+        custom: {
+          ...existingTheme.custom,
+          isLowContrast: contrastMode === "lowContrast",


I thought we were just going to return contrastMode here and it would be a string of type "highContrast" | "lowContrast"?

I was using this at one point but am now just exporting odysseyContrastMode

bryancunningham-okta · 2024-09-25T18:10:16Z

packages/odyssey-react-mui/src/BackgroundContext.tsx

@@ -0,0 +1,142 @@
+/*!


In keeping with best practices, this file should be named <something>Provider.

updated it to ContrastModeProvider.

also removes the confusing disabled button storybook story.

issue likely from adding ContrastContext and needing a delay for some tests.

bryancunningham-okta · 2024-09-30T18:42:16Z

packages/odyssey-react-mui/src/OdysseyProvider.tsx

+const OdysseyProvider = <SupportedLanguages extends string>(
+  props: OdysseyProviderProps<SupportedLanguages>,
+) => (
+  <ContrastModeProvider>


@benlister-okta You can't currently set an explicit contrast mode. Looking at the code it seems like something you were trying to support

You can by passing contrastMode="lowContrast" (or highContrast) to the provider. The logic for it is in the ContrastProvider file: https://github.com/okta/odyssey/pull/2364/files#diff-69196edb1faa4f2d8100e2344e3ebd81a965621255c029d9380c8a5f976d2901

in this block useLayoutEffect(() => { if (explicitContrastMode) { setContrastMode(explicitContrastMode); } else { const isLowContrast = parentBackgroundColor === Tokens.HueNeutral50; setContrastMode(isLowContrast ? "lowContrast" : "highContrast"); } }, [parentBackgroundColor, explicitContrastMode]);

Right, but you are not passing that prop through here. I'd expect OdysseyProvider to take contrastMode prop and that be passed through to ContrastModeProvider here

ahh, I see what you mean now, I added it in there now too. I was testing in storybook by importing directly so I didn't notice.

bryancunningham-okta · 2024-09-30T18:43:52Z

packages/odyssey-react-mui/src/index.ts

@@ -59,6 +59,7 @@ export { useOdysseyDesignTokens } from "./OdysseyDesignTokensContext";
 export * from "./Accordion";
 export * from "./Autocomplete";
 export { badgeContentMaxValues } from "./Badge";
+export * from "./ContrastModeProvider";


Nit: Alphabetize :)

bryancunningham-okta

Looks good. Couple comments but nothing major

bryancunningham-okta · 2024-09-30T18:48:07Z

Does this effectively make the OdysseyThemeProvider useless?

bryancunningham-okta · 2024-09-30T18:49:37Z

packages/odyssey-react-mui/src/OdysseyProvider.tsx

-          {children}
-        </OdysseyTranslationProvider>
-      </ScopedCssBaseline>
-    </OdysseyThemeProvider>


@benlister-okta I'm not sure we can remove the OdysseyThemeProvider here to be honest

I guess you are passing the tokens along to OdysseyDesignTokensContext here, so, maybe we can

Yes we should still retain the functionality since the theme-related props (designTokensOverride, shadowRootElement, themeOverride) are now passed directly to OdysseyThemeProvider to simplify things.

I'm not sure what you mean by this.^ We are not using OdysseyThemeProvider at all now

I meant it was implementing the same functionality. I see what you mean though, it no longer uses OdysseyThemeProvider at all so it would be weird and confusing to have the functionality duplicated.

I just updated to revert back to using OdysseyThemeProvider and extend it with the contrast stuff

bryancunningham-okta · 2024-09-30T20:25:38Z

packages/odyssey-react-mui/src/OdysseyProvider.tsx

+};
+
+const OdysseyProvider = <SupportedLanguages extends string>(
+  props: OdysseyProviderProps<SupportedLanguages>,


Little nit-picky but Id probably do this.

Suggested change

props: OdysseyProviderProps<SupportedLanguages>,

{ contrastMode, ...providerProps }: OdysseyProviderProps<SupportedLanguages>,

thanks, updated with this.

bryancunningham-okta · 2024-09-30T20:29:28Z

packages/odyssey-react-mui/src/OdysseyProvider.tsx

+        designTokensOverride={designTokensOverride}
+        shadowDomElement={shadowDomElement}
+        shadowRootElement={shadowRootElement}
+        themeOverride={{


We'll probably want to memoize this object we're passing to the themeOverride

good call, I added it in here now.

bryancunningham-okta · 2024-09-30T20:38:31Z

packages/odyssey-react-mui/src/OdysseyProvider.tsx

@@ -60,6 +60,25 @@ const OdysseyProviderInner = <SupportedLanguages extends string>({
 }: OdysseyProviderProps<SupportedLanguages>) => {
  const { contrastMode } = useContrastContext();

+  const memoizedThemeProps = useMemo(


Sorry, i just meant the override styles themselves. Something like this:

const memoizedThemeProps = useMemo( () => ({ ...themeOverride, odysseyContrastMode: contrastMode, }), [ themeOverride, contrastMode, ], );

But, your solution is probably fine too

got it! I like your suggestion, its easier to read, IMO

benlister-okta mentioned this pull request Sep 20, 2024

[DRAFT] Add BackgroundContext for gray background component theme changes #2362

Closed

1 task

benlister-okta commented Sep 20, 2024

View reviewed changes

KevinGhadyani-Okta reviewed Sep 23, 2024

View reviewed changes

benlister-okta changed the title ~~[DRAFT - OdysseyProvider] Add BackgroundContext for gray background component theme changes~~ Add BackgroundContext for gray background component theme changes Sep 25, 2024

benlister-okta marked this pull request as ready for review September 25, 2024 18:00

benlister-okta requested a review from a team as a code owner September 25, 2024 18:00

bryancunningham-okta reviewed Sep 25, 2024

View reviewed changes

benlister-okta added 18 commits September 29, 2024 15:06

fix: add context for background color

f0598ac

fix: add backgroundprovider to tag

6728a3e

fix: update Banner border to match Figma

3f4d335

also removes the confusing disabled button storybook story.

fix: add backgroundContext for Status

04cf532

fix: move backgroundContext to OdysseyProvider

e5d0797

fix: ts issues, match tests with other pr

2942d58

fix: import error, color fix

0a5ac98

fix: remove comment

c8c2bab

fix: update tests

4557369

fix: update tests

2872dcf

fix: backgroundcontext auto bg detection

dd4d5b0

fix: refactor

af68a7c

fix: css formatting

287fbcf

fix: rename provider: Background to ContextMode

aaaa11b

fix: refactor, color fix

52933da

fix: interaction tests

5d1014f

fix: adjust contrast

3fedeed

fix: fixes for failing interaction tests

edffd62

benlister-okta force-pushed the bl_gray_bg_components-theme branch from 4f65e67 to edffd62 Compare September 29, 2024 22:08

fix: fixing interaction tests

84a596d

issue likely from adding ContrastContext and needing a delay for some tests.

bryancunningham-okta reviewed Sep 30, 2024

View reviewed changes

bryancunningham-okta previously approved these changes Sep 30, 2024

View reviewed changes

bryancunningham-okta reviewed Sep 30, 2024

View reviewed changes

fix: pr feedback

274d568

benlister-okta dismissed bryancunningham-okta’s stale review via 274d568 September 30, 2024 19:06

fix: re-implement OdysseyThemeProvider

a3824d0

bryancunningham-okta reviewed Sep 30, 2024

View reviewed changes

fix: pr feedback

d187fc3

bryancunningham-okta reviewed Sep 30, 2024

View reviewed changes

fix: pr feedback, add memo

58ce765

bryancunningham-okta reviewed Sep 30, 2024

View reviewed changes

fix: update memo use

e9dc095

bryancunningham-okta approved these changes Oct 1, 2024

View reviewed changes

oktapp-aperture-okta bot merged commit 09ba964 into main Oct 1, 2024
3 checks passed

oktapp-aperture-okta bot deleted the bl_gray_bg_components-theme branch October 1, 2024 16:10

	props: OdysseyProviderProps<SupportedLanguages>,
	{ contrastMode, ...providerProps }: OdysseyProviderProps<SupportedLanguages>,

Add BackgroundContext for gray background component theme changes #2364

Add BackgroundContext for gray background component theme changes #2364

Conversation

benlister-okta commented Sep 20, 2024 • edited by jordankoschei-okta Loading

Summary

Testing & Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlister-okta Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryancunningham-okta Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryancunningham-okta left a comment

Choose a reason for hiding this comment

bryancunningham-okta commented Sep 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryancunningham-okta Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlister-okta commented Sep 20, 2024 •

edited by jordankoschei-okta

Loading

benlister-okta Sep 20, 2024 •

edited

Loading

bryancunningham-okta Sep 30, 2024 •

edited

Loading

bryancunningham-okta Sep 30, 2024 •

edited

Loading