refactor(misc): CSS harcoded variables and classes replaced with react-tokens and react-styles #9266

adamviktora · 2023-06-13T08:28:02Z

What: Closes #9116

Notes, further related issues and some questions I have:

CSS values were replaced with tokens in all .tsx files and .md demos, which include parts of TS code.
CSS values were not replaced in .css and .md files.
I was replacing mainly strings containing pf-v5 prefix, there are still plenty of them (usually some modifiers) starting with just pf- that could also be replaced.
I was not able to replace values in any of the integration tests in packages/react-integration/cypress/integration as the react-tokens and react-styles packages cannot be imported due to configuration of package.json -- looking at it right now
These CSS variables don't have a react token:

--pf-v5-c-about-modal-box--BackgroundImage - issue Bug - About modal - use token instead for hard coded value #9246
--pf-v5-c-slider__step--Left
--pf-v5-global--default-color--200 - this one should probably be --pf-v5-global--Color--200 ?

There are lots of CSS classes which are not directly included in react-styles and I had to compute them from parts of string like this for example: "pf-v5-c-code-editor__keyboard-shortcuts" -> {'${styles.codeEditor}__keyboard-shortcuts'}

patternfly-build · 2023-06-13T08:33:30Z

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

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

… linter

…with --no-verify

this has to be done, because token names must preserve its original name, so the docs framework don't throw an error

nicolethoen

This PR is going to conflict with @kmcfaul's #9392. Where she is cleaning up usage of the screen reader class names. It may be worth either leaving it to katie to handle all those classes to make sure you don't undo anything she's doing - or make the same changes in your PR if hers merges first.

packages/react-core/src/components/Alert/examples/Alert.md

packages/react-core/src/components/Badge/__tests__/Badge.test.tsx

packages/react-core/src/components/Badge/Badge.tsx

nicolethoen · 2023-08-03T14:14:46Z

packages/react-core/src/components/DatePicker/__tests__/__snapshots__/DatePicker.test.tsx.snap

yeah, this file sort of demonstrates the difference between the hardcoded class and the one imported from utilities. I don't think we want that.

...onents/MultipleFileUpload/__tests__/__snapshots__/MultipleFileUploadStatusItem.test.tsx.snap

tlabaj · 2023-08-31T14:38:07Z

packages/react-code-editor/src/components/CodeEditor/CodeEditor.tsx

@@ -647,7 +648,9 @@ class CodeEditor extends React.Component<CodeEditorProps, CodeEditorState> {
                  {...getRootProps({
                    onClick: (event) => event.stopPropagation() // Prevents clicking TextArea from opening file dialog
                  })}
-                  className={`pf-v5-c-file-upload ${isDragActive && 'pf-m-drag-hover'} ${isLoading && 'pf-m-loading'}`}
+                  className={`${fileUploadStyles.fileUpload} ${isDragActive && fileUploadStyles.modifiers.dragHover} ${


@mcoker shouldn't these styles be available in the code editor stylesheet rather than also needing to load file upload stylesheet?

@tlabaj good question! Nope we don't include the file upload component styles in the code editor. AFAIK we don't do that for any component - a component only includes its styles. As far as I can tell, the only reason file upload styles are being imported is to get the loading state, which is not supported in the code editor. I don't think the way we're applying the file upload styles here is ideal though, I'm surprised it works the way it is 😅 I suppose it's worth considering if we need the loading styles at all? And if so, I suppose we should add them to the code editor. It should be a really simple addition.

We have since introduced the multiple file upload component, which mimics the empty state in its presentation. I wonder if that might be a more suitable component to use here instead of the empty state for uploading a file?

@mcoker I will open a follow up issue to reconsider this implementation

tlabaj · 2023-08-31T14:41:01Z

...ges/react-core/src/components/AboutModal/examples/AboutModalComplexUserPositionedContent.tsx

@@ -24,12 +25,12 @@ export const AboutModalComplexUserPositionedContent: React.FunctionComponent = (
        hasNoContentContainer={true}
        productName="Product Name"
      >
-        <TextContent id="test1" className="pf-v5-u-py-xl">
+        <TextContent id="test1" className={spacing.pyXl}>


I think it might be more meaningful for the consumer to see the actual class name in the example.
@mcoker what do you think?

I have an issue in org to improve our documentation around react-tokens and react-styles so that products can better utilize them. I'd personally advocate for demonstrating how to use them well since this will reduce the work required to migrate to new major versions of PF. This avoids the hardcoding of strings with versioned prefixes.

tlabaj · 2023-08-31T14:45:10Z

packages/react-core/src/components/AboutModal/AboutModalBoxContent.tsx

@@ -19,7 +19,7 @@ export const AboutModalBoxContent: React.FunctionComponent<AboutModalBoxContentP
  ...props
 }: AboutModalBoxContentProps) => (
  <div className={css(styles.aboutModalBoxContent)} {...props}>
-    <div className={css('pf-v5-c-about-modal-box__body')}>
+    <div className={css(`${styles.aboutModalBox}__body`)}>


I do not think this change is needed. THe reason the class is hard coded here is because it is a placeholder and has no associated styles

The benefit to making changes like this one, as the versioned prefixes change over time, we won't need to come back and change all these hard coded strings - they will update automatically.

tlabaj · 2023-08-31T14:49:11Z

packages/react-core/src/components/Accordion/__tests__/AccordionContent.test.tsx

@@ -85,14 +86,14 @@ test('Renders with inherited element props spread to the component', () => {
  expect(screen.getByRole('heading')).toHaveAccessibleName('Label');
 });

-test('Renders with class name pf-v5-c-accordion__expandable-content', () => {
+test(`Renders with class name ${styles.accordionExpandableContent}`, () => {


@wise-king-sullyman, what do you think about updating the test like this? Will loading the stylesheets impact performance?

It might impact performance, I'm honestly not sure if it would really be noticeable or not.

I think it's worth trying though, if we can't do this or something like it we'll have to update all of the classnames each major release, which isn't ideal.

Probably minimal. They job looks like it still completes in 4 minutes.

tlabaj · 2023-08-31T14:57:01Z

packages/react-core/src/components/Drawer/examples/DrawerStackedContentBodyElements.tsx

@@ -30,7 +31,7 @@ export const DrawerStackedContentBodyElements: React.FunctionComponent = () => {
  const panelContent = (
    <DrawerPanelContent>
      <DrawerHead>
-        <h3 className="pf-v5-c-title pf-m-2xl" tabIndex={isExpanded ? 0 : -1} ref={drawerRef}>
+        <h3 className={`${styles.title} ${styles.modifiers['2xl']}`} tabIndex={isExpanded ? 0 : -1} ref={drawerRef}>


Same comment here about this not being meaningful.
Can we just use the title component here @mcoker ?

either that, or the styles object should be updated to explicitly note that we are loading Title styles and not drawer styles as I might assume from reading this quickly.

OK, I will use the Title component

tlabaj · 2023-08-31T14:58:49Z

packages/react-core/src/components/DualListSelector/DualListSelectorControl.tsx

@@ -40,7 +41,7 @@ export const DualListSelectorControlBase: React.FunctionComponent<DualListSelect
  const privateRef = React.useRef(null);
  const ref = innerRef || privateRef;
  return (
-    <div className={css('pf-v5-c-dual-list-selector__controls-item', className)} {...props}>
+    <div className={css(`${styles.dualListSelectorControls}-item`, className)} {...props}>


same comment about this not being needed. This applies to everywhere this change was made.

I still want to push back on this. I think this is a pretty good improvement.
The benefit to making changes like this one, as the versioned prefixes change over time, we won't need to come back and change all these hard coded strings - they will update automatically.

this may be in the styles now since there is a rule here

https://github.com/patternfly/patternfly/blob/f600d12ef6c7d135daaa5a1b3d8520210137dd88/src/patternfly/components/DualListSelector/dual-list-selector.scss#L302-L304

nicolethoen

This is looking really good to me!
The only change I'd still encourage is to make sure that the Drawer examples that are loading Title styles be clear which styles they are using. the styles object should be called titleStyles or something similar to be clearer.

tlabaj · 2023-09-01T13:25:29Z

@adamviktora I opened the following issues for the Slider token
#9563
patternfly/patternfly#5892

tlabaj · 2023-09-01T13:26:32Z

@mcoker can you please answer Adam's question about this

--pf-v5-global--default-color--200 - this one should probably be --pf-v5-global--Color--200 ?

mcoker · 2023-09-05T15:39:47Z

Hey @adamviktora re:

--pf-v5-global--default-color--200 - this one should probably be --pf-v5-global--Color--200 ?

The equivalent of that one in v5 is --pf-v5-global--custom-color--200. That said, if you're referring to this example below, unless the point of it being custom is that you can change the color, we could just remove the custom color and leave the text as "Custom item" - that should be obvious enough? wdyt @tlabaj?

patternfly-react/packages/react-core/src/components/Wizard/examples/WizardWithCustomNavItem.tsx

Line 13 in aad1b1b

    
           content: <span style={{ color: 'var(--pf-v5-global--default-color--200)' }}>Custom item</span>

mcoker

This LGTM! I left some nit comments, but even if you want to make them, I think they should be in a separate PR. Not sure if I could re-review this one 😅

The only one I'm not sure if it needs to be updated is a place you're no longer using .join() in a var assignment - #9266 (comment). You included .join() other places you made that same update to the button classNames but not that one.

mcoker · 2023-10-09T22:49:22Z

packages/react-core/src/components/Drawer/examples/Drawer.md

@@ -18,6 +18,9 @@ propComponents:
 section: components
 ---

+import styles from '@patternfly/react-styles/css/components/Title/title';


just a nit but should this be namespaced like titleStyles or something since this is the drawer component?

mcoker · 2023-10-09T22:51:38Z

packages/react-core/src/components/DualListSelector/DualListSelectorControl.tsx

@@ -40,7 +41,7 @@ export const DualListSelectorControlBase: React.FunctionComponent<DualListSelect
  const privateRef = React.useRef(null);
  const ref = innerRef || privateRef;
  return (
-    <div className={css('pf-v5-c-dual-list-selector__controls-item', className)} {...props}>
+    <div className={css(`${styles.dualListSelectorControls}-item`, className)} {...props}>


this may be in the styles now since there is a rule here

https://github.com/patternfly/patternfly/blob/f600d12ef6c7d135daaa5a1b3d8520210137dd88/src/patternfly/components/DualListSelector/dual-list-selector.scss#L302-L304

mcoker · 2023-10-09T23:00:00Z

packages/react-core/src/components/Modal/examples/Modal.md

@@ -10,6 +10,8 @@ import WarningTriangleIcon from '@patternfly/react-icons/dist/esm/icons/warning-
 import CaretDownIcon from '@patternfly/react-icons/dist/esm/icons/caret-down-icon';
 import BullhornIcon from '@patternfly/react-icons/dist/esm/icons/bullhorn-icon';
 import HelpIcon from '@patternfly/react-icons/dist/esm/icons/help-icon';
+import spacing from '@patternfly/react-styles/css/utilities/Spacing/spacing';
+import styles from '@patternfly/react-styles/css/components/Form/form';


should we import this as formStyles?

mcoker · 2023-10-09T23:07:26Z

packages/react-core/src/components/Tabs/__tests__/OverflowTab.test.tsx

@@ -376,7 +377,7 @@ test('Passes Popper an appendTo value of the presentation element', async () =>
  await user.click(screen.getByRole('tab'));

  // This assertion relies on the structure of the Popper mock to verify the correct props are being sent to Popper
-  expect(screen.getByText('Append to class name: pf-v5-c-tabs__item pf-m-overflow')).toBeVisible();
+  expect(screen.getByText(`Append to class name: ${styles.tabsItem} pf-m-overflow`)).toBeVisible();


should this use the styles.modifiers object?

Suggested change

expect(screen.getByText(`Append to class name: ${styles.tabsItem} pf-m-overflow`)).toBeVisible();

expect(screen.getByText(`Append to class name: ${styles.tabsItem} ${styles.modifiers.overflow}`)).toBeVisible();

about the styles.modifiers we can do this in a separate PR, there is a ton of pf-m-something classes in the codebase, but as they don't use the -v5- version, I don't know if is it necessary

Cool, sounds good! There could be a future where we version modifiers, and if we used styles.modifiers, it would pull that in. IMO it's probably better to not hard code as much as we can, too.

mcoker · 2023-10-09T23:08:54Z

packages/react-core/src/components/TextInputGroup/__tests__/TextInputGroupMain.test.tsx

@@ -180,7 +181,7 @@ describe('TextInputGroupMain', () => {
    expect(hintInput).toBeInTheDocument();
  });

-  it('renders the hint input with classes pf-v5-c-text-input-group__text-input and pf-m-hint', () => {
+  it(`renders the hint input with classes ${styles.textInputGroupTextInput} and pf-m-hint`, () => {


should this use the styles.modifiers obj?

Suggested change

it(`renders the hint input with classes ${styles.textInputGroupTextInput} and pf-m-hint`, () => {

it(`renders the hint input with classes ${styles.textInputGroupTextInput} and ${styles.modifiers.hint}`, () => {

mcoker · 2023-10-09T23:42:29Z

packages/react-core/src/demos/examples/JumpLinks/JumpLinksWithDrawer.js

@@ -20,6 +20,7 @@ import {
  getResizeObserver
 } from '@patternfly/react-core';
 import DashboardWrapper from '@patternfly/react-core/src/demos/examples/DashboardWrapper';
+import styles from '@patternfly/react-styles/css/components/Masthead/masthead';


Suggested change

import styles from '@patternfly/react-styles/css/components/Masthead/masthead';

import mastheadStyles from '@patternfly/react-styles/css/components/Masthead/masthead';

mcoker · 2023-10-09T23:43:53Z

packages/react-integration/demo-app-ts/src/components/demos/AlertGroupDemo/AlertGroupDemo.tsx

+  InputGroupItem
+} from '@patternfly/react-core';
+import { css } from '@patternfly/react-styles';
+import styles from '@patternfly/react-styles/css/components/Button/button';


Suggested change

import styles from '@patternfly/react-styles/css/components/Button/button';

import buttonStyles from '@patternfly/react-styles/css/components/Button/button';

mcoker · 2023-10-09T23:45:35Z

packages/react-integration/demo-app-ts/src/components/demos/AlertGroupDemo/AlertGroupDemo.tsx

@@ -36,7 +45,7 @@ export class AlertGroupDemo extends React.Component<{}, AlertGroupDemoState> {
      this.setState({ alerts: [...this.state.alerts, ...incomingAlerts] });
    };
    const getUniqueId = () => new Date().getTime();
-    const btnClasses = ['pf-v5-c-button', 'pf-m-secondary'].join(' ');
+    const btnClasses = css(styles.button, styles.modifiers.secondary);


you use .join() in other places where you're doing this. is that needed?

Suggested change

const btnClasses = css(styles.button, styles.modifiers.secondary);

const btnClasses = css(styles.button, styles.modifiers.secondary).join(' ');

I am using the css function here, which is a wrapper which "joins args into a className string".
So [styles.button, styles.modifiers.secondary].join(' '); is the same as css(styles.button, styles.modifiers.secondary).

We could probably use the css(classnames) function everywhere, instead of [classnames].join(" "), but the result is the same.

mcoker · 2023-10-09T23:45:50Z

...ion/demo-app-ts/src/components/demos/ChipGroupDemo/ChipGroupWithOverflowChipEventHandler.tsx

@@ -1,5 +1,8 @@
 import React, { Component } from 'react';
 import { Chip, ChipGroup } from '@patternfly/react-core';
+import { css } from '@patternfly/react-styles';
+import styles from '@patternfly/react-styles/css/components/Title/title';


Suggested change

import styles from '@patternfly/react-styles/css/components/Title/title';

import titleStyles from '@patternfly/react-styles/css/components/Title/title';

mcoker · 2023-10-09T23:51:37Z

packages/react-table/src/components/Table/Td.tsx

@@ -2,6 +2,7 @@ import * as React from 'react';
 import { css } from '@patternfly/react-styles';
 import styles from '@patternfly/react-styles/css/components/Table/table';
 import scrollStyles from '@patternfly/react-styles/css/components/Table/table-scrollable';
+import stylesTreeView from '@patternfly/react-styles/css/components/Table/table-tree-view';


Suggested change

import stylesTreeView from '@patternfly/react-styles/css/components/Table/table-tree-view';

import treeViewStyles from '@patternfly/react-styles/css/components/Table/table-tree-view';

adamviktora · 2023-10-11T13:43:04Z

@mcoker thanks for the review! I did some changes in this PR, but they can be viewed separately in this commit: 85850e8 (nothing crucial though, mostly renaming styles -> buttonStyles and similar changes)

mcoker

🎃👍

patternfly-build · 2023-10-12T16:56:08Z

Your changes have been released in:

@patternfly/react-code-editor@5.2.0-prerelease.6
@patternfly/react-core@5.2.0-prerelease.6
@patternfly/react-docs@6.2.0-prerelease.6
@patternfly/react-integration@5.1.1-prerelease.11
demo-app-ts@5.1.1-prerelease.29
@patternfly/react-table@5.2.0-prerelease.6

Thanks for your contribution! 🎉

adamviktora requested review from nicolethoen and tlabaj June 13, 2023 08:29

adamviktora marked this pull request as draft June 13, 2023 08:30

adamviktora added 3 commits June 21, 2023 10:18

refactor(misc): replace hardcoded css variables with tokens

0e906c9

refactor(misc): replace hardcoded CSS classes with react-styles

c84b0c7

test(misc): update snapshots to use pf-v5-u-screen-reader

846cb48

adamviktora force-pushed the css-hardcoded-fix branch from 3f52947 to 846cb48 Compare June 21, 2023 08:33

adamviktora added 6 commits June 21, 2023 12:28

fix(demos): proper imports in .md files

a88b0f5

refactor(token names): rename tokens to pass build:docs framework and…

bdb2910

… linter

refactor(token names): rename tokens to pass yarn docs:build, commit …

e522621

…with --no-verify

refactor: use whole imports to not blow up CJS bundle sizes

d95501d

fix: eslint-disable camel case

190e2fb

this has to be done, because token names must preserve its original name, so the docs framework don't throw an error

feat(misc): add imports to .md demos to pass docs build

124f14a

adamviktora marked this pull request as ready for review June 22, 2023 14:43

tlabaj added the css review label Aug 2, 2023

tlabaj requested a review from mcoker August 2, 2023 20:23

nicolethoen reviewed Aug 3, 2023

View reviewed changes

adamviktora and others added 5 commits August 31, 2023 12:02

fix: use pf-v5-screen-reader class

a925b4b

Merge branch 'main' into css-hardcoded-fix

92e93a7

fix: update test snapshots

87b5ed3

formatted with prettier

be766f5

refactor(Alert): rename styles to buttonStyles

1850fb2

adamviktora requested a review from nicolethoen August 31, 2023 13:36

tlabaj requested changes Aug 31, 2023

View reviewed changes

nicolethoen reviewed Sep 1, 2023

View reviewed changes

docs(Drawer): update Drawer example to use Title

f1c55ca

adamviktora and others added 3 commits September 20, 2023 11:12

Merge branch 'main' into css-hardcoded-fix

6652e2b

docs(Wizard): remove custom color in custom nav item example

c60983b

lint error fix

5e1c040

tlabaj approved these changes Sep 21, 2023

View reviewed changes

Merge branch 'main' into css-hardcoded-fix

b0ce03e

mcoker approved these changes Oct 9, 2023

View reviewed changes

adamviktora and others added 2 commits October 11, 2023 14:27

Merge branch 'main' into css-hardcoded-fix

30b579d

refactor(misc): rename styles and other imports

85850e8

nicolethoen approved these changes Oct 12, 2023

View reviewed changes

mcoker approved these changes Oct 12, 2023

View reviewed changes

tlabaj merged commit e054f9e into patternfly:main Oct 12, 2023
10 checks passed

renovate bot mentioned this pull request May 26, 2024

fix(deps): update shared components packages (major) redhat-developer/app-services-guides#580

Open

1 task

	expect(screen.getByText(`Append to class name: ${styles.tabsItem} pf-m-overflow`)).toBeVisible();
	expect(screen.getByText(`Append to class name: ${styles.tabsItem} ${styles.modifiers.overflow}`)).toBeVisible();

	it(`renders the hint input with classes ${styles.textInputGroupTextInput} and pf-m-hint`, () => {
	it(`renders the hint input with classes ${styles.textInputGroupTextInput} and ${styles.modifiers.hint}`, () => {

	import styles from '@patternfly/react-styles/css/components/Masthead/masthead';
	import mastheadStyles from '@patternfly/react-styles/css/components/Masthead/masthead';

	import styles from '@patternfly/react-styles/css/components/Button/button';
	import buttonStyles from '@patternfly/react-styles/css/components/Button/button';

	const btnClasses = css(styles.button, styles.modifiers.secondary);
	const btnClasses = css(styles.button, styles.modifiers.secondary).join(' ');

	import styles from '@patternfly/react-styles/css/components/Title/title';
	import titleStyles from '@patternfly/react-styles/css/components/Title/title';

	import stylesTreeView from '@patternfly/react-styles/css/components/Table/table-tree-view';
	import treeViewStyles from '@patternfly/react-styles/css/components/Table/table-tree-view';

refactor(misc): CSS harcoded variables and classes replaced with react-tokens and react-styles #9266

refactor(misc): CSS harcoded variables and classes replaced with react-tokens and react-styles #9266

Conversation

adamviktora commented Jun 13, 2023 • edited

patternfly-build commented Jun 13, 2023 • edited

nicolethoen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolethoen left a comment

Choose a reason for hiding this comment

tlabaj commented Sep 1, 2023

tlabaj commented Sep 1, 2023

mcoker commented Sep 5, 2023

mcoker left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamviktora Oct 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamviktora commented Oct 11, 2023

mcoker left a comment

Choose a reason for hiding this comment

patternfly-build commented Oct 12, 2023

adamviktora commented Jun 13, 2023 •

edited

patternfly-build commented Jun 13, 2023 •

edited

mcoker left a comment •

edited

adamviktora Oct 11, 2023 •

edited