Skip to content
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

Merged
merged 8 commits into from
May 21, 2024

Conversation

kmcfaul
Copy link
Contributor

@kmcfaul kmcfaul commented May 16, 2024

What: Closes #10314, #10262, #10335

  • Updated core version to alpha.135
  • Updated generateTokens, removing old, unused files it was pulling in to map vars
  • Updated Text component suite to use default content classes (Add support for classes on TextContent children #10262)
  • Updated Wizard to not use danger token directly & structure update (Update wizard to remove reference to error color token #10335)
  • Added blurb to docs about Text changes
  • Removed dropdown styles usage from Menu
  • Updated Charts font weight token global_FontWeight_bold to global_font_weight_heading_bold
  • Updated Slider inset token c_slider__step_Left to c_slider__step_InsetInlineStart
  • Updated Td/Th inset tokens c_table__sticky_cell_Left and c_table__sticky_cell_Right to c_table__sticky_cell_InsetInlineStart and c_table__sticky_cell_InsetInlineEnd
  • Updated Toolbar chip group class (variant prop still uses chip-group, but maps to pf-m-label-group now)
  • Updated Wizard Footer to use ActionList (also for deprecated)
  • Updated global height breakpoint token names
  • Updated snapshots
  • Fixed integration tests
  • Updated Table integration demo token global_primary_color_100 to global_color_brand_default
  • Fixed Table transformers test referencing dropdown styles to menu styles
  • Fixed react-token test, updated old token to new token

@kmcfaul
Copy link
Contributor Author

kmcfaul commented May 16, 2024

I'm not sure if it's expected that a bunch of charts colors changed in the snapshots @mcoker Thoughts?

@patternfly-build
Copy link
Contributor

patternfly-build commented May 16, 2024

Copy link
Contributor

@mcoker mcoker left a 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.

Copy link
Contributor

@mcoker mcoker left a 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-core/src/demos/examples/Card/CardStatus.tsx Outdated Show resolved Hide resolved
packages/react-core/src/demos/examples/Card/CardStatus.tsx Outdated Show resolved Hide resolved
packages/react-tokens/README.md 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"
Copy link
Contributor

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.

<div {...props} className={css(styles.notificationDrawerHeader, className)}>
<Text component={TextVariants.h1} className={css(styles.notificationDrawerHeaderTitle)}>
{title}
</Text>

Copy link
Contributor Author

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

packages/react-core/src/components/Wizard/WizardToggle.tsx Outdated Show resolved Hide resolved
@kmcfaul
Copy link
Contributor Author

kmcfaul commented May 20, 2024

@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?

@kmcfaul
Copy link
Contributor Author

kmcfaul commented May 20, 2024

@patternfly/design-reviewers @patternfly/core-in-react-reviewers This PR should include the core updates to the Select outlined in #10011. If you don't mind using the surge here to re-verify the Select we can get that issue closed here too.

Edit: The core PR is actually still up, disregard this note.

Copy link
Contributor

@mcoker mcoker left a 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/components/Wizard/WizardToggle.tsx Outdated Show resolved Hide resolved
<CheckCircleIcon color={global_success_color_100.var} />
<CheckCircleIcon color={global_color_status_success_default.var} />
Copy link
Contributor

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?

Copy link
Contributor Author

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,
Copy link
Contributor

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?

Copy link
Contributor Author

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).

Copy link
Contributor

@thatblindgeye thatblindgeye left a 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

@tlabaj
Copy link
Contributor

tlabaj commented May 21, 2024

@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?

@kmcfaul Lets open a follow up for this for now.

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@tlabaj tlabaj merged commit 1f1b93c into patternfly:v6 May 21, 2024
13 checks passed
@patternfly-build
Copy link
Contributor

Your changes have been released in:

  • @patternfly/react-charts@8.0.0-alpha.21
  • @patternfly/react-code-editor@6.0.0-alpha.59
  • @patternfly/react-core@6.0.0-alpha.59
  • @patternfly/react-docs@7.0.0-alpha.62
  • @patternfly/react-drag-drop@6.0.0-alpha.40
  • @patternfly/react-icons@6.0.0-alpha.21
  • @patternfly/react-integration@6.0.0-alpha.30
  • demo-app-ts@5.1.1-alpha.58
  • @patternfly/react-styles@6.0.0-alpha.21
  • @patternfly/react-table@6.0.0-alpha.59
  • @patternfly/react-templates@6.0.0-alpha.9
  • @patternfly/react-tokens@6.0.0-alpha.21

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants