-
Notifications
You must be signed in to change notification settings - Fork 353
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
chore(deps): Bump v6 branch to latest core alpha #9774
Conversation
Preview: https://patternfly-react-pr-9774.surge.sh A11y report: https://patternfly-react-pr-9774-a11y.surge.sh |
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.
just some nits, copy/paste error in multiple places :)
it'd be good to double check that any global tokens in the core repo are getting translated appropriately into react-tokens, just like the other css variables. |
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.
Notification badges don't look quite right - there's a weird outline and the hover is wrong. But these are also slightly still in flight along with some button work from @mcoker
Page is getting a scrollbar that isn't there in core. That might take a little more investigation.
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 is really cool!
couple quick questions:
I see that we are defining the token for blue_100 for example as:
export const _pf_t_color_blue_100 = {
"name": "--pf-t--color--blue--100",
"value": "#d9e9ff",
"var": "var(--pf-t--color--blue--100)"
};
export default _pf_t_color_blue_100;
Will that eventually be renamed and somehow replace the coexisting global variable?:
export const global_palette_blue_100 = {
"name": "--pf-v5-global--palette--blue-100",
"value": "#bee1f4",
"var": "var(--pf-v5-global--palette--blue-100)"
};
export default global_palette_blue_100;
EDIT: this might be exactly what your TODO in the PR description is referring to - so never mind :)
Also, the individual token files look good, but in patternfly_variables.js I see quite a few tokens that have value: undefined
such as:
"_pf_t_global_color_nonstatus_orangered_100": {
"name": "--pf-t--global--color--nonstatus--orangered--100",
"value": "undefined",
"values": [
"--pf-t--color--orangered--200",
"undefined"
]
},
Is that expected?
@srambach This can be addressed by the issue for notification badge #9811 |
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
What: Closes #9411
Additional issues: towards #9811, #9808, and #9809
backgroundColor
prop since light background is now the default with no option to change it..pf-v5-c-notification-badge__count
no longer has a style associated with it so I updated the code with hardcoded placeholder string.theme
prop since it only supports the default theme.light
,dark
and `darker variant since the are no longer supported.isSelected
prop since it is no longer supported in PageScreenshots of tokens BEFORE:
Screenshots of tokens AFTER: