-
Notifications
You must be signed in to change notification settings - Fork 36
Remove SearchField border on gray background #2464
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
Conversation
…highContrast style
renderFieldComponent={renderFieldComponent} | ||
/> | ||
<StyledContainerToEnsureThemeProviderDivIsFullWidth> | ||
<OdysseyThemeProvider> |
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.
out of personal curiosity: is it not required that folks have an <OdysseyProvider>
(which includes <OdysseyThemeProvider>
) wrapping their usage of this?
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.
Maybe a better question: is this the new pattern for contrast-aware components, that they have to include an <OdysseyThemeProvider>
to enable that?
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.
@benschell-okta Yes, adding the OdysseyThemeProvider
here scopes the ContrastModeProvider
down to this component's parent background color. Ideally this would be more intuitive/be a different provider. Something that at a glance you can tell why it's there/what it's doing fo ryou
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.
I believe, In it's current state, without adding this provider here, the 'awareness' would be based on the background color of the html
element
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.
That last point was the exact kernel I needed. This localizes the context that is computed within and that makes perfect sense
const StyledContainerToEnsureThemeProviderDivIsFullWidth = styled("div")({ | ||
width: "100%", | ||
}); | ||
|
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.
We discussed possible other solutions for this. Not 100% sure the solution, but one is putting width: "inherit"
on OdysseyThemeProvider
's div
, and another is putting a flex-basis: "100%"
on this one.
…highContrast style
DES-7007
Summary
Testing & Screenshots