-
Notifications
You must be signed in to change notification settings - Fork 359
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
feat(misc, TextContent): update core version & text content updates #10378
Conversation
I'm not sure if it's expected that a bunch of charts colors changed in the snapshots @mcoker Thoughts? |
Preview: https://patternfly-react-pr-10378.surge.sh A11y report: https://patternfly-react-pr-10378-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.
This looks great! A comment on the wizard footer action list. Looks like the structure between <WizardFooter>
and <WizardFooterInternal>
are different, and unless my eyes are playing tricks on me, I think they're both different from core. I'd expect that to be:
wizard-footer
action-list
action-list-group
action-list-item // back
action-list-item // next
action-list-group
action-list-item // cancel
I think that should future-proof from needing HTML changes if we add more destructive and/or non-destructive actions in the future. I can elaborate more on that if you'd like.
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.
@kmcfaul left some feedback. I think it's fine as-is if we want to merge it and follow up.
Re: the charts colors, I didn't review them all but I believe that's to be expected. We can review in depth with design but that can happen after this merges IMO.
packages/react-table/src/components/Table/examples/TableMisc.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/demos/examples/Card/CardAggregateStatus.tsx
Outdated
Show resolved
Hide resolved
@@ -6,7 +6,7 @@ exports[`NotificationDrawerHeader drawer header with count applied 1`] = ` | |||
class="pf-v6-c-notification-drawer__header" | |||
> | |||
<h1 | |||
class="pf-v6-c-notification-drawer__header-title" | |||
class="pf-v6-c-content--h1 pf-v6-c-notification-drawer__header-title" |
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.
Any idea why we used <Text>
here? It would have previously just generated an unstyled <h1>
(unless wrapped in <TextContent>
, which it doesn't look like it is), but now it's going to get a bunch of styles from the text/content component which we don't want AFAIK.
patternfly-react/packages/react-core/src/components/NotificationDrawer/NotificationDrawerHeader.tsx
Lines 40 to 43 in 4b8789a
<div {...props} className={css(styles.notificationDrawerHeader, className)}> | |
<Text component={TextVariants.h1} className={css(styles.notificationDrawerHeaderTitle)}> | |
{title} | |
</Text> |
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.
Removed the Text wrapper for a plain h1
@tlabaj Noticed that we had one last usage of deprecated SelectOption in a deprecated Table editable cell demo. I updated it to use the new SelectOption, but it looks like that demo doesn't open the select menu. Do we want to open a follow up for this? Or are we planning on eventually removing deprecated Table in v6? |
Edit: The core PR is actually still up, disregard this note. |
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 one small change needed for the wizard toggle.
Also left some comments on card demo icons but those are all pre-existing and just nits, so no importance to get in with this PR IMO.
Also there is a line in the react-tokens readme we could probably clean up -
import global_BackgroundColor_100 from '@patternfly/react-tokens/dist/esm/global_-background-color_100'; |
^ references an old global var name and the filename in the import has hyphens that should be underscores. We could probably just copy/paste the import
line from the block below it there:
import global_background_color_primary_default from '@patternfly/react-tokens/dist/esm/global_background_color_primary_default';
packages/react-core/src/demos/examples/Card/CardAggregateStatus.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/demos/examples/Card/CardAggregateStatus.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/demos/examples/Card/CardAggregateStatus.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/demos/examples/Card/CardAggregateStatus.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/demos/examples/Card/CardAggregateStatus.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/demos/examples/Card/CardAggregateStatus.tsx
Outdated
Show resolved
Hide resolved
<CheckCircleIcon color={global_success_color_100.var} /> | ||
<CheckCircleIcon color={global_color_status_success_default.var} /> |
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.
any reason we wouldn't use <Icon/>
for the icons in this file, too?
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.
Nope I don't think so, I will update them here too. Any time I think we don't have to directly use tokens we probably should.
@@ -80,7 +80,8 @@ export const ToolbarItem: React.FunctionComponent<ToolbarItemProps> = ({ | |||
<div | |||
className={css( | |||
styles.toolbarItem, | |||
variant && styles.modifiers[toCamel(variant) as 'pagination' | 'label' | 'chipGroup'], | |||
variant && styles.modifiers[toCamel(variant) as 'pagination' | 'label'], | |||
variant === ToolbarItemVariant['chip-group'] && styles.modifiers.labelGroup, |
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.
Could we instead update the chip-group
variant to label-group
for the ToolbarItemVariant enum and prop type?
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.
Opened #10405 to track that change in a follow up, along with the rest of Toolbar (as there are several usages of "chip" in its language for prop/component names).
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.
Above comment isn't a blocker and can be tackled in a followup
@kmcfaul Lets open a follow up for this for now. |
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
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #10314, #10262, #10335
alpha.135
global_FontWeight_bold
toglobal_font_weight_heading_bold
c_slider__step_Left
toc_slider__step_InsetInlineStart
c_table__sticky_cell_Left
andc_table__sticky_cell_Right
toc_table__sticky_cell_InsetInlineStart
andc_table__sticky_cell_InsetInlineEnd
chip-group
, but maps topf-m-label-group
now)global_primary_color_100
toglobal_color_brand_default