-
Notifications
You must be signed in to change notification settings - Fork 375
fix(deps): updated Core for v5 versioning #9075
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
fix(deps): updated Core for v5 versioning #9075
Conversation
|
Preview: https://patternfly-react-pr-9075.surge.sh A11y report: https://patternfly-react-pr-9075-a11y.surge.sh |
dgutride
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.
One minor fix to table - should we land first?
packages/react-table/src/components/Table/utils/decorators/treeRow.tsx
Outdated
Show resolved
Hide resolved
8967033 to
7d75ab3
Compare
| @@ -127,7 +127,7 @@ To add additional components and information to a group, you may use the followi | |||
|
|
|||
| ### Centered section | |||
|
|
|||
| By default, a page section spans the width of the page. To reduce the width of a section, use the `isWidthLimited` property. To center align width-limited page sections, use the `isCenterAligned` property. When the main content area of a page is wider than the value of a centered, width-limited page section's `--pf-c-page--section--m-limit-width--MaxWidth` custom property, the section will automatically be centered. | |||
| By default, a page section spans the width of the page. To reduce the width of a section, use the `isWidthLimited` property. To center align width-limited page sections, use the `isCenterAligned` property. When the main content area of a page is wider than the value of a centered, width-limited page section's `--pf-v5-c-page--section--m-limit-width--MaxWidth` custom property, the section will automatically be centered. | |||
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.
nice catch!
| @@ -297,7 +297,7 @@ const SearchInputBase: React.FunctionComponent<SearchInputProps> = ({ | |||
| <TextInputGroupUtilities> | |||
| {resultsCount && <Badge isRead>{resultsCount}</Badge>} | |||
| {!!onNextClick && !!onPreviousClick && ( | |||
| <div className="pf-c-text-input-group__group"> | |||
| <div className="pf-v5-c-text-input-group__group"> | |||
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.
One day we will need to look deeper into why classnames like this one are hardcoded. but not as part of this.
| 'pf-c-tree-view__node-toggle' | ||
| ); | ||
| const isExpandable = | ||
| activeElement?.firstElementChild?.firstElementChild?.classList.contains('pf-c-tree-view__node-toggle'); |
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.
does this need to be updated?
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.
Yep this should be updated to pf-v5-c-tree-view__node-toggle as part of a follow up
| @@ -35,7 +35,7 @@ const ValidationProgress: React.FunctionComponent<ValidationProgressProps> = ({ | |||
| }, [tick]); | |||
|
|
|||
| return ( | |||
| <div className="pf-l-bullseye"> | |||
| <div className="pf-v5-l-bullseye"> | |||
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.
ooof there are a lot of hardcoded classnames in these wizards... i feel like that's a red flag, but not something to address now. just making notes.
| @@ -14,7 +14,7 @@ function getCSSClasses(cssString) { | |||
| * @param {string} className - Class name | |||
| */ | |||
| function formatClassName(className) { | |||
| return camelcase(className.replace(/pf-((c|l|m|u|is|has)-)?/g, '')); | |||
| return camelcase(className.replace(/pf-(v5-)?((c|l|m|u|is|has)-)?/g, '')); | |||
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.
🎉
| grid-template-areas: | ||
| 'actions-main actions-extra' | ||
| 'main main'; | ||
| row-gap: var(--pf-v5-global--spacer--md); |
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'm going to make a notes to investigate whether this console CSS is still needed by the extension. Ideally, it got moved out with the react-console extension into its own repo.
| --pf-l-toolbar__group--MarginLeft: var(--pf-global--spacer--xl); | ||
| --pf-l-toolbar__item--MarginRight: var(--pf-global--spacer--md); | ||
| --pf-l-toolbar__item--MarginLeft: var(--pf-global--spacer--md); } | ||
| --pf-v5-l-toolbar__section--MarginTop: var(--pf-v5-global--spacer--md); |
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.
Also adding to my list to investigate if this layout is used by anyone anymore...
| @@ -6,7 +6,7 @@ const { readFileSync } = require('fs'); | |||
| const pfStylesDir = dirname(require.resolve('@patternfly/patternfly/patternfly.css')); | |||
|
|
|||
| // Helpers | |||
| const formatCustomPropertyName = key => key.replace('--pf-', '').replace(/-+/g, '_'); | |||
| const formatCustomPropertyName = (key) => key.replace('--pf-v5-', '').replace(/-+/g, '_'); | |||
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.
🎉
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.
I'll approve so we can unblock broken workspaces. We can do follow up clean up as need be.
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #8816
Integration tests skipped:
The Slider and Toolbar tests I skipped because they're failing after I updated them for a similar failure, so just skipping them for now to get checks to pass.
Additional issues: