task(settings): Support additional layout customization#19926
task(settings): Support additional layout customization#19926vpomerleau merged 1 commit intomainfrom
Conversation
| } | ||
|
|
||
| function calculateGradientContrast(gradient: string): string { | ||
| function calculateGradientContrast( |
There was a problem hiding this comment.
I like that your other functions have jsdoc comments. Can we add them across the board?
| featureFlags?: FeatureFlagsRelierCmsInfo | undefined; | ||
| favicon?: string | undefined; | ||
| // Note: The backend may return this as an enum, but the values are compatible strings | ||
| headlineFontSize?: HeadlineFontSize | string | null; |
There was a problem hiding this comment.
This seems to break the pattern by using | null instead of | undefined. Is there a reason for this?
There was a problem hiding this comment.
null aligns with the generated types in libs/shared/cms, though I'm not sure why others here have undefined
|
|
||
| export const CmsHeaderComparison = () => ( | ||
| <AppLayout> | ||
| <div style={{ display: 'flex', flexDirection: 'column', gap: '40px' }}> |
There was a problem hiding this comment.
My only nit about all these extra inline styles is I hope folks see this as a storybook only sorta thing (well and PDFs), and if we change the design system values we won't be changing these, but I don't necessarily have a better suggestion since I know Tailwind's not looking at these files and nor should we want it to so 🤷♀️ In a perfect world we could create reusable Storybook components.
(Nothing to do here, I'm just mumbling to myself 😄)
|
|
||
| /* For gradient buttons, maintain the gradient on active state */ | ||
| .cta-primary-cms.cta-gradient:active:not([aria-disabled='true']) { | ||
| background-color: transparent !important; |
There was a problem hiding this comment.
Looking at this again, I decided to tweak hover/active styling to add contrast for gradient background and removed !important. It looks like the specificity is high enough that it will take priority.
| const textColorClass = getTextColorClassName(buttonColor, 'maximum'); | ||
| // Map textColorClass to actual CSS color value | ||
| const mappedTextColor = | ||
| TEXT_COLOR_CSS_VALUES[textColorClass] || 'rgb(0, 0, 0)'; |
There was a problem hiding this comment.
Should the fallback be our text-grey-900 instead of text-black? (Maybe not)
There was a problem hiding this comment.
Ah yes, good call. I'll use that as the standard dark contrast option
Because: * We want to allow setting a gradient as button colour * We want to allow customization of headline text size and color This commit: * Update the CmsButtonWithFallback component to support setting a gradient as buttonColor * Update calculate-contrast to provide better contrast on light colour gradient * Update cta styling to prevent a border artifact when background is gradient * Support headlineFontSize and headlineTextColor attributes from cmsInfo Closes #FXA-12791
LZoog
left a comment
LZoog
left a comment
There was a problem hiding this comment.
Paired in Zoom for testing locally. LGTM!!
3 participants
Because
This pull request
Issue that this pull request solves
Closes: FXA-12791
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Example medium headline font size with blue text and gradient button
Button styling examples from storybook with automated text color adjustments based on contrast with button colour.
Other information (Optional)
Testing requires:
dcdb5ae7add825d2) with entrypoint (e.g.,fxa_avatar_menu)