fix(Multiple): update old component classnames #9139

gitdallas · 2023-05-17T17:44:46Z

Possibly closes or atleast makes effort towards #9116 . I replaced hard coded classes in instances that the style object had one to match.

patternfly-build · 2023-05-17T17:54:24Z

Preview: https://patternfly-react-pr-9139.surge.sh

A11y report: https://patternfly-react-pr-9139-a11y.surge.sh

wise-king-sullyman · 2023-05-17T20:07:04Z

Should these be changed?

wise-king-sullyman · 2023-05-17T20:23:08Z

Primary detail icons are still off. This PR is left, v4 is right. Is this change expected?

wise-king-sullyman

I'm still curious about my previous questions, but I wouldn't block for either of them.

gitdallas · 2023-05-17T20:40:24Z

@wise-king-sullyman Ah - I think I just searched the react-core/component dir.. I'll see if I can update those real quick and see what's going on with the icons.

gitdallas · 2023-05-17T21:26:30Z

@mcoker @srambach @mattnolting

any idea about the spacing of icons? for v5 i'm seeing https://patternfly-react-v5.surge.sh/patterns/primary-detail/react-demos/primary-detail-full-page/ :

and for v4 i'm seeing https://www.patternfly.org/v4/demos/primary-detail/react-demos/primary-detail-full-page/ :

the v5 margin-right is coming up as nothing

mcoker · 2023-05-17T21:48:15Z

@gitdallas @wise-king-sullyman nice find! That's a bug in core - your update looks good to me. I created a core issue to fix that, no react follow up should be needed patternfly/patternfly#5591

gitdallas · 2023-05-17T22:32:51Z

@mcoker if you could give an approval with your "approval", that'd be great 😆

tlabaj · 2023-05-18T00:32:19Z

@gitdallas @wise-king-sullyman nice find! That's a bug in core - your update looks good to me. I created a core issue to fix that, no react follow up should be needed patternfly/patternfly#5591

@mcoker that is a PR approval correct?

mcoker

LGTM! I see some others that should probably be updated, too? But looks like they're all example things, so no rush for release candidate.

Components

packages/react-core//src/components/MenuToggle/examples/MenuToggle.css:#ws-react-c-menu-toggle-primary .pf-c-menu-toggle, #ws-react-c-menu-toggle-secondary .pf-c-menu-toggle {
packages/react-core//src/components/Alert/examples/alert.css:.ws-react-c-alert .pf-c-alert {
packages/react-core//src/components/Checkbox/examples/checkbox.css:.ws-react-c-checkbox .pf-c-check.nested {
packages/react-core//src/demos/JumpLinks.md:    const masthead = document.getElementsByClassName('pf-c-masthead')[0];
packages/react-core//src/demos/examples/JumpLinks/JumpLinksWithDrawer.js:    const masthead = document.getElementsByClassName('pf-c-masthead')[0];

Layouts

packages/react-core//src/layouts/Split/examples/split.css:.ws-react-l-split .pf-l-split,
packages/react-core//src/layouts/Split/examples/split.css:.ws-react-l-split .pf-l-split > .pf-l-split__item {
packages/react-core//src/layouts/Gallery/examples/gallery.css:.ws-react-l-gallery .pf-l-gallery {
packages/react-core//src/layouts/Gallery/examples/gallery.css:.ws-react-l-gallery .pf-l-gallery > div {
packages/react-core//src/layouts/Bullseye/examples/bullseye.css:.ws-react-l-bullseye .pf-l-bullseye,
packages/react-core//src/layouts/Bullseye/examples/bullseye.css:.ws-react-l-bullseye .pf-l-bullseye > div {
packages/react-core//src/layouts/Level/examples/level.css:.ws-react-l-level .pf-l-level {
packages/react-core//src/layouts/Level/examples/level.css:.ws-react-l-level .pf-l-level,
packages/react-core//src/layouts/Level/examples/level.css:.ws-react-l-level .pf-l-level > div {
packages/react-core//src/layouts/Stack/examples/stack.css:.ws-react-l-stack > .pf-l-stack {
packages/react-core//src/layouts/Stack/examples/stack.css:.ws-react-l-stack .pf-l-stack,
packages/react-core//src/layouts/Stack/examples/stack.css:.ws-react-l-stack .pf-l-stack > .pf-l-stack__item {
packages/react-core//src/layouts/Flex/__tests__/Flex.test.tsx:            .className.replace('pf-l-flex', '')
packages/react-core//src/layouts/Flex/examples/flex.css:.ws-react-l-flex .pf-l-flex {
packages/react-core//src/layouts/Flex/examples/flex.css:.ws-react-l-flex .pf-l-flex > div,
packages/react-core//src/layouts/Grid/examples/grid.css:.ws-react-l-grid > .pf-l-grid,
packages/react-core//src/layouts/Grid/examples/grid.css:.ws-react-l-grid > .pf-l-grid > .pf-l-grid__item {
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">100C</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">50C</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">0C</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">3.6Ghz</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">1.5Ghz</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">0GHZ</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">100C</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">50C</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">0C</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">3.6Ghz</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">1.5Ghz</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">0GHZ</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">100C</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">50C</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">0C</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">3.6Ghz</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">1.5Ghz</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">0GHZ</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">100C</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">50C</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">0C</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">3.6Ghz</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">1.5Ghz</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">0GHZ</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">100C</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">50C</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">0C</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">3.6Ghz</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">1.5Ghz</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">0GHZ</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">100C</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">50C</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">0C</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">3.6Ghz</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">1.5Ghz</div>
packages/react-core//src/demos/CardDemos.md:                          <div className="pf-l-flex__item">0GHZ</div>

nicolethoen · 2023-05-18T01:37:18Z

@mcoker @tlabaj
i opened a follow up issue and added it to the clean up examples/demo epic on the roadmap

tlabaj

@dlabrecq does this will need to be updated correct? I can open a follow up to this PR.

dlabrecq · 2023-05-18T15:59:05Z

@tlabaj charts uses tokens, so snapshots will need updating. However, the only "pf-" prefixes are for a couple keys and a chart pattern ID.

tlabaj

It looks like some updates are needed in react-styles and anywhere those classes are being pulled in. the classes her are still using pf-c prefix. The Popper should at least be updated here and we can follow up with the rest. They may require some investigation.
https://github.com/patternfly/patternfly-react/tree/v5/packages/react-styles/src/css/components

mcoker · 2023-05-18T18:18:18Z

re: the react-styles updates @tlabaj mentioned, I'm not sure where the names for the color vars are coming from - https://github.com/patternfly/patternfly-react/blob/v5/packages/react-styles/src/css/components/Table/inline-edit.css

We don't have a --global--[color]--number CSS var in core, though it looks like that is how the vars were defined before we added the v5 prefix, too - 3879fc7#diff-d99fb7d33fdb9c29c555e2926969439a2857b3ec7e95c13155c6c21bb3dce1e2

I can't find an editable table example using composable table, but I see the legacy table example - https://www.patternfly.org/v4/components/table/react-legacy#editable-rows. It doesn't look like any of the classnames in https://github.com/patternfly/patternfly-react/blob/v5/packages/react-styles/src/css/components/Table/inline-edit.css are being applied in edit mode.

If those colors still need to be applied, --pf-v5-global--[color]--[number] should be --pf-v5-global--palette--[color]-[number] - and the color names should be lower-case.

nicolethoen · 2023-05-18T18:18:34Z

@tlabaj I think that can be a follow up issue because there are lots of other problems with that CSS.

tlabaj · 2023-05-18T20:32:04Z

@tlabaj charts uses tokens, so snapshots will need updating. However, the only "pf-" prefixes are for a couple keys and a chart pattern ID.

@dlabrecq sorry i forgot to paste a link to what I was talking about above. I assume this is what needs to be updated correct?
https://github.com/patternfly/patternfly-react/blob/v5/packages/react-charts/src/components/ChartUtils/chart-helpers.ts

mcoker

popper class lgtm 👍

tlabaj · 2023-05-18T21:07:08Z

Here is a list of issues that will be follow ups to this PR
#9144, #9150, #9151

* fix(Multiple): update old component classnames * update cypress test selectors * fix cypress selectors * fix cypress selectors * update some code-editor/deprecated/table/integration directories * update popper css --------- Co-authored-by: nicolethoen <nthoen@redhat.com>

gitdallas requested review from nicolethoen, thatblindgeye and wise-king-sullyman May 17, 2023 17:49

tlabaj added the css review label May 17, 2023

tlabaj requested a review from mcoker May 17, 2023 20:28

wise-king-sullyman approved these changes May 17, 2023

View reviewed changes

mcoker mentioned this pull request May 17, 2023

Flex - nested layout spacer issues patternfly/patternfly#5591

Closed

thatblindgeye approved these changes May 17, 2023

View reviewed changes

mcoker approved these changes May 18, 2023

View reviewed changes

tlabaj self-requested a review May 18, 2023 13:46

tlabaj reviewed May 18, 2023

View reviewed changes

tlabaj requested changes May 18, 2023

View reviewed changes

gitdallas and others added 6 commits May 18, 2023 16:14

fix(Multiple): update old component classnames

4ec37e0

update cypress test selectors

b0e7315

fix cypress selectors

a22df9e

fix cypress selectors

8cafb34

update some code-editor/deprecated/table/integration directories

931fdaf

update popper css

d570b79

nicolethoen force-pushed the fix/menu/css-spacing-issue branch from 93c8786 to d570b79 Compare May 18, 2023 20:17

tlabaj mentioned this pull request May 18, 2023

Bug - react-styles - class names need to be updated #9150

Closed

nicolethoen approved these changes May 18, 2023

View reviewed changes

mcoker approved these changes May 18, 2023

View reviewed changes

tlabaj approved these changes May 18, 2023

View reviewed changes

tlabaj merged commit 8fbd940 into patternfly:v5 May 18, 2023

fix(Multiple): update old component classnames #9139

fix(Multiple): update old component classnames #9139

Uh oh!

Conversation

gitdallas commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wise-king-sullyman commented May 17, 2023

Uh oh!

wise-king-sullyman commented May 17, 2023

Uh oh!

wise-king-sullyman left a comment

Choose a reason for hiding this comment

Uh oh!

gitdallas commented May 17, 2023

Uh oh!

gitdallas commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcoker commented May 17, 2023

Uh oh!

gitdallas commented May 17, 2023

Uh oh!

tlabaj commented May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Components

Layouts

Uh oh!

nicolethoen commented May 18, 2023

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

dlabrecq commented May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlabaj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcoker commented May 18, 2023

Uh oh!

nicolethoen commented May 18, 2023

Uh oh!

tlabaj commented May 18, 2023

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

tlabaj commented May 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

gitdallas commented May 17, 2023 •

edited

Loading

patternfly-build commented May 17, 2023 •

edited

Loading

gitdallas commented May 17, 2023 •

edited

Loading

tlabaj commented May 18, 2023 •

edited

Loading

dlabrecq commented May 18, 2023 •

edited

Loading

tlabaj left a comment •

edited

Loading