-
Notifications
You must be signed in to change notification settings - Fork 147
docs(dark-theme-handbook): Updates content for v6. #3893
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
mcoker
left a comment
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.
LGTM! Left a few comments as feedback, but IMO it's good the way it is, too.
packages/documentation-site/patternfly-docs/content/developer-resources/dark-theme-handbook.md
Outdated
Show resolved
Hide resolved
packages/documentation-site/patternfly-docs/content/developer-resources/dark-theme-handbook.md
Outdated
Show resolved
Hide resolved
nicolethoen
left a comment
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.
This update references a pf-v6-theme-dark which I don't think exists, yet. Right, @srambach ?
It actually does exist in our Brand component, though not quite yet as the main switch for light/dark. |
srambach
left a comment
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 left a couple of comments, but they are minor and could certainly be updated later if others agree.
| * When making overrides, utilize the global and component CSS variable system to define the override. For example, to override a primary button’s background color, declare `.pf-v6-c-button { --pf-v6-c-button--m-primary--BackgroundColor: [color]; }` instead of `.pf-v6-c-button.pf-m-primary { background-color: [color]; }`. | ||
| * **Use PatternFly components without customizations.** Most problems with dark theming are due to the use of custom styles, overrides to PatternFly styles, and non-PatternFly components. To ensure consistent styling and behavior when switching themes, try to use PatternFly components as they are. | ||
|
|
||
| * **Use tokens to define any overrides.** |
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'd change this to something like
| * **Use tokens to define any overrides.** | |
| * **Use tokens and component variables to define any overrides.** |
| * **Use PatternFly components without customizations.** Most problems with dark theming are due to the use of custom styles, overrides to PatternFly styles, and non-PatternFly components. To ensure consistent styling and behavior when switching themes, try to use PatternFly components as they are. | ||
|
|
||
| * **Use tokens to define any overrides.** | ||
| * For example, to override a primary button’s background color, declare `.pf-[version]-c-button { --pf-[version]-c-button--m-primary--BackgroundColor: [color]; }` instead of `.pf-[version]-c-button.pf-m-primary { background-color: [color]; }`. |
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.
It might be good to say [color token] instead of [color] - it's technically fine but at first read it seems to say that assigning it to a hex code would be fine as long as you use the component variable.
|
@nicolethoen @mcoker @srambach hi, this is ready for final review! |
|
@srambach @nicolethoen re the use of |
|
I think it will be |
Closes #3890