-
Notifications
You must be signed in to change notification settings - Fork 357
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
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
Changes from 14 commits
0e906c9
c84b0c7
846cb48
a88b0f5
bdb2910
e522621
d95501d
190e2fb
124f14a
a925b4b
92e93a7
87b5ed3
be766f5
1850fb2
f1c55ca
6652e2b
c60983b
5e1c040
b0ce03e
30b579d
85850e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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`)}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
{hasNoContentContainer ? children : <div className={css(contentStyles.content)}>{children}</div>} | ||
</div> | ||
<p className={css(styles.aboutModalBoxStrapline)}>{trademark}</p> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import React from 'react'; | ||
import { AboutModal, Alert, Button, TextContent, TextList, TextListItem } from '@patternfly/react-core'; | ||
import brandImg from '../../assets/brandImg.svg'; | ||
import spacing from '@patternfly/react-styles/css/utilities/Spacing/spacing'; | ||
|
||
export const AboutModalComplexUserPositionedContent: React.FunctionComponent = () => { | ||
const [isModalOpen, setIsModalOpen] = React.useState(false); | ||
|
@@ -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}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it might be more meaningful for the consumer to see the actual class name in the example. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
<h4>About</h4> | ||
<p>Content here</p> | ||
</TextContent> | ||
<Alert variant="info" title="Updates available" /> | ||
<TextContent id="test2" className="pf-v5-u-py-xl"> | ||
<TextContent id="test2" className={spacing.pyXl}> | ||
<TextList component="dl"> | ||
<TextListItem component="dt">CFME Version</TextListItem> | ||
<TextListItem component="dd">5.5.3.4.20102789036450</TextListItem> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ import { render, screen } from '@testing-library/react'; | |
|
||
import { AccordionContent } from '../AccordionContent'; | ||
import { AccordionContext } from '../AccordionContext'; | ||
import styles from '@patternfly/react-styles/css/components/Accordion/accordion'; | ||
|
||
jest.mock('../AccordionExpandableContentBody', () => ({ | ||
AccordionExpandableContentBody: ({ children }) => <div aria-label="Expanded content body mock">{children}</div> | ||
|
@@ -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}`, () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wise-king-sullyman, what do you think about updating the test like this? Will loading the stylesheets impact performance? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably minimal. They job looks like it still completes in 4 minutes. |
||
render( | ||
<AccordionContext.Provider value={{ ContentContainer: 'h3' }}> | ||
<AccordionContent>Test</AccordionContent> | ||
</AccordionContext.Provider> | ||
); | ||
|
||
expect(screen.getByRole('heading')).toHaveClass('pf-v5-c-accordion__expandable-content'); | ||
expect(screen.getByRole('heading')).toHaveClass(styles.accordionExpandableContent); | ||
}); | ||
|
||
test('Renders with custom class names provided via prop', () => { | ||
|
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.
@mcoker shouldn't these styles be available in the code editor stylesheet rather than also needing to load file upload stylesheet?
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.
@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?
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.
@mcoker I will open a follow up issue to reconsider this implementation