fix(icons): removing inline styles from SVG icons #5275

wojta · 2020-12-17T16:20:23Z

What:
Closes #4767

How

removing inline style with vertical-align: -0.125em;
replacing with CSS style

Additional issues:
patternfly/patternfly#3774

wojta · 2020-12-17T16:33:17Z

packages/react-styles/src/css/base/patternfly-svg.css

@@ -0,0 +1,5 @@
+@charset "UTF-8";
+
+.pf-svg-vertical-align {


I don't know exactly where to put this CSS class. It would require manually importing this file in the every application for this to work properly.

Yes, this belongs in https://github.com/patternfly/patternfly or existing applications will not get the styles.

Edit: I'd ask @mcoker where to put it. I suppose you could add it to the base styles.

@wojta we have a global PF class stylesheet here where you could add this class https://github.com/patternfly/patternfly/blob/master/src/patternfly/base/_common.scss. Let me know if you'd like me to create the issue/PR.

@mcoker added there patternfly/patternfly#3775

patternfly-build · 2020-12-17T16:39:13Z

PF4 preview: https://patternfly-react-pr-5275.surge.sh
A11y report: https://patternfly-react-pr-5275-a11y.surge.sh

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

codecov-io · 2021-01-13T23:44:07Z

Codecov Report

Merging #5275 (f733e11) into master (c26b549) will decrease coverage by 0.05%.
The diff coverage is 46.15%.

@@            Coverage Diff             @@
##           master    #5275      +/-   ##
==========================================
- Coverage   51.91%   51.85%   -0.06%     
==========================================
  Files         579      579              
  Lines       11272    11321      +49     
  Branches     4184     4201      +17     
==========================================
+ Hits         5852     5871      +19     
- Misses       4683     4710      +27     
- Partials      737      740       +3

Flag	Coverage Δ
patternfly4	`51.85% <46.15%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../components/ChartUtils/chart-interactive-legend.ts	`10.00% <0.00%> (-0.26%)`	⬇️
...de-editor/src/components/CodeEditor/CodeEditor.tsx	`25.00% <0.00%> (ø)`
...es/react-core/src/components/DataList/DataList.tsx	`24.36% <0.00%> (-0.64%)`	⬇️
...ages/react-core/src/components/Tooltip/Tooltip.tsx	`48.35% <0.00%> (-1.10%)`	⬇️
...ore/src/components/Wizard/WizardFooterInternal.tsx	`100.00% <ø> (ø)`
packages/react-icons/src/createIcon.tsx	`73.91% <0.00%> (-1.09%)`	⬇️
...t-table/src/components/Table/utils/headerUtils.tsx	`95.00% <ø> (ø)`
...-core/src/components/Drawer/DrawerPanelContent.tsx	`30.70% <29.62%> (-1.34%)`	⬇️
...ages/react-core/src/components/Spinner/Spinner.tsx	`63.63% <33.33%> (-36.37%)`	⬇️
...ckages/react-core/src/components/Select/Select.tsx	`40.57% <42.85%> (+0.31%)`	⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1e8e28...f733e11. Read the comment docs.

mcoker

I'm not seeing the styles applied - looks like this needs a core bump?

wojta · 2021-01-14T00:25:47Z

Yes, it needs newer @patternfly/patternfly. I'm not sure how is this done properly.

tlabaj · 2021-01-19T18:51:05Z

The core version has been updated. You can rebase this PR now to get the styles

wojta · 2021-01-19T21:27:07Z

Rebased, demos look good, styles are there now.

tlabaj · 2021-02-03T18:03:02Z

this one has a conflict. Can you please rebase again.

mcoker · 2021-02-08T17:13:54Z

Everything looks good visually, though a couple of discrepancies I see that I'd like to confirm:

On the icons page, the vertical-align value is different. The inline style is currently -0.375em, while the .pf-svg-vertical-align class is fixed at -0.125em. However, the icons page is the only place I've seen anything other than -0.125em used as the vertical align value.
Spot checking a few components, it looks like we're currently applying vertical-align inline on svg's, yet in this PR, we aren't applying the .pf-svg-vertical-align class. Examples are empty state, button, and data-list.

redallen

Since you put 'pf-svg-vertical-align' in the common styles there's no to introduce @patternfly/react-styles as a dependency. Someone might want to use icons without styles.

packages/react-icons/src/createIcon.tsx

redallen

Sorry I missed @mcoker 's comment yesterday!

packages/react-icons/src/createIcon.tsx

nicolethoen · 2023-03-29T17:16:10Z

I added an update to this issue: #8794 to make sure the react-icons read me is updated. Once CI passes here, we can get this merged.

jenny-s51

CI is passing - ready for review @nicolethoen !

srambach · 2023-03-31T18:57:20Z

packages/react-core/src/components/FileUpload/examples/FileUploadCustomPreview.tsx

@@ -29,7 +29,7 @@ export const CustomPreviewFileUpload: React.FunctionComponent = () => {
    >
      {value && (
        <div className="pf-u-m-md">
-          <FileUploadIcon size="lg" /> Custom preview here for your {value.size}-byte file named {value.name}
+          <FileUploadIcon width="2em" height="2em" /> Custom preview here for your {value.size}-byte file named {value.name}


I'm not sure why this change, but I'm also not a React dev :-)

The size prop was removed in createIcon.tsx and the default is now small, so I had to set this custom styling here to match the styling we had before @srambach - not sure if there is a better way to do this

Ok, I see that now. 🤷‍♀️

srambach · 2023-03-31T19:05:23Z

packages/react-core/src/components/FileUpload/examples/FileUploadCustomPreview.tsx

@@ -29,7 +29,7 @@ export const CustomPreviewFileUpload: React.FunctionComponent = () => {
    >
      {value && (
        <div className="pf-u-m-md">
-          <FileUploadIcon size="lg" /> Custom preview here for your {value.size}-byte file named {value.name}
+          <FileUploadIcon width="2em" height="2em" /> Custom preview here for your {value.size}-byte file named {value.name}


Ok, I see that now. 🤷‍♀️

mcoker

Nice job @jenny-s51! 🚀

tlabaj

LGTM Jenny!
Do we have a code mod issue for this?

jenny-s51

@tlabaj we do now 🙂 patternfly/pf-codemods#370

tlabaj

@jenny-s51 We can merge this one once it is rebased

patternfly-build · 2023-04-06T18:43:42Z

Your changes have been released in:

@patternfly/react-code-editor@5.0.0-alpha.59
@patternfly/react-core@5.0.0-alpha.58
@patternfly/react-docs@6.0.0-alpha.63
@patternfly/react-icons@5.0.0-alpha.9
demo-app-ts@5.0.0-alpha.42
@patternfly/react-integration@5.0.0-alpha.20
@patternfly/react-table@5.0.0-alpha.59

Thanks for your contribution! 🎉

According to the PatternFly codemod this no longer is valid, it still works however but `orange` is not the same as `--pf-v5-global--warning-color--100` so this was wrong anyway. patternfly/patternfly-react#5275

According to the PatternFly codemod this no longer is valid, it still works however but `orange` is not the same as `--pf-v5-global--warning-color--100` so this was wrong anyway. patternfly/patternfly-react#5275 Cherry-picked from main commit f0ba89d

wojta requested a review from redallen December 17, 2020 16:27

wojta commented Dec 17, 2020

View reviewed changes

tlabaj added the Do Not Merge label Dec 17, 2020

wojta mentioned this pull request Jan 6, 2021

Add class with vertical-align for some SVG images to be properly aligned patternfly/patternfly#3774

Closed

wojta force-pushed the issue-4767-breadcrumbs-dividers branch from c4fa837 to adf71fd Compare January 8, 2021 15:16

tlabaj assigned jschuler Jan 11, 2021

KKoukiou mentioned this pull request Jan 13, 2021

Icon offset override should only apply to icons, not all SVGs cockpit-project/cockpit#15132

Closed

wojta force-pushed the issue-4767-breadcrumbs-dividers branch from adf71fd to a8ff3fa Compare January 13, 2021 16:19

wojta requested a review from mcoker January 14, 2021 00:01

mcoker reviewed Jan 14, 2021

View reviewed changes

wojta changed the title ~~[WIP] breaking change: fix/removing inline styles from SVG icons~~ breaking change: fix/removing inline styles from SVG icons Jan 15, 2021

wojta force-pushed the issue-4767-breadcrumbs-dividers branch from d444ac8 to 6831af9 Compare January 15, 2021 20:20

tlabaj removed the Do Not Merge label Jan 19, 2021

wojta force-pushed the issue-4767-breadcrumbs-dividers branch from 6831af9 to 1615259 Compare January 19, 2021 20:31

wojta force-pushed the issue-4767-breadcrumbs-dividers branch from 1615259 to f733e11 Compare February 3, 2021 21:39

redallen suggested changes Feb 8, 2021

View reviewed changes

packages/react-icons/src/createIcon.tsx Outdated Show resolved Hide resolved

packages/react-icons/src/createIcon.tsx Outdated Show resolved Hide resolved

wojta force-pushed the issue-4767-breadcrumbs-dividers branch from f733e11 to 0585aab Compare February 9, 2021 09:18

wojta requested a review from redallen February 9, 2021 09:39

redallen suggested changes Feb 9, 2021

View reviewed changes

packages/react-icons/src/createIcon.tsx Outdated Show resolved Hide resolved

wojta force-pushed the issue-4767-breadcrumbs-dividers branch 4 times, most recently from 72c68aa to 6bc0771 Compare February 9, 2021 17:09

jenny-s51 force-pushed the issue-4767-breadcrumbs-dividers branch 2 times, most recently from 8b2e529 to be96b98 Compare March 29, 2023 13:26

nicolethoen mentioned this pull request Mar 29, 2023

Contribution guides / readmes are out of date #8794

Closed

14 tasks

jenny-s51 requested review from kmcfaul and nicolethoen March 30, 2023 13:16

jenny-s51 reviewed Mar 30, 2023

View reviewed changes

jenny-s51 requested a review from mcoker March 31, 2023 15:48

nicolethoen approved these changes Mar 31, 2023

View reviewed changes

gitdallas approved these changes Mar 31, 2023

View reviewed changes

thatblindgeye approved these changes Mar 31, 2023

View reviewed changes

srambach reviewed Mar 31, 2023

View reviewed changes

srambach approved these changes Mar 31, 2023

View reviewed changes

mcoker approved these changes Mar 31, 2023

View reviewed changes

tlabaj approved these changes Apr 3, 2023

View reviewed changes

jenny-s51 mentioned this pull request Apr 3, 2023

Misc icons - removed size, noVerticalAlign, and color props patternfly/pf-codemods#370

Closed

jenny-s51 reviewed Apr 3, 2023

View reviewed changes

tlabaj reviewed Apr 5, 2023

View reviewed changes

jenny-s51 force-pushed the issue-4767-breadcrumbs-dividers branch 2 times, most recently from a2fcbd8 to 9fde82a Compare April 5, 2023 14:07

fix conflict

0cae3eb

jenny-s51 force-pushed the issue-4767-breadcrumbs-dividers branch from 9fde82a to 0cae3eb Compare April 5, 2023 15:08

nicolethoen merged commit 1dfb69d into patternfly:v5 Apr 6, 2023
10 checks passed

KKoukiou mentioned this pull request Apr 24, 2023

Bug - [*Icon] - [Icons from react-icon (not the Icon component)] do not match any of the size options from Icon #8990

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(icons): removing inline styles from SVG icons #5275

fix(icons): removing inline styles from SVG icons #5275

wojta commented Dec 17, 2020 •

edited

wojta Dec 17, 2020

redallen Jan 4, 2021 •

edited

mcoker Jan 4, 2021

wojta Jan 6, 2021 •

edited

patternfly-build commented Dec 17, 2020 •

edited

codecov-io commented Jan 13, 2021 •

edited

mcoker left a comment

wojta commented Jan 14, 2021 •

edited

tlabaj commented Jan 19, 2021

wojta commented Jan 19, 2021

tlabaj commented Feb 3, 2021

mcoker commented Feb 8, 2021

redallen left a comment

redallen left a comment

nicolethoen commented Mar 29, 2023

jenny-s51 left a comment

srambach Mar 31, 2023

jenny-s51 Mar 31, 2023 •

edited

srambach Mar 31, 2023

srambach Mar 31, 2023

mcoker left a comment

tlabaj left a comment

jenny-s51 left a comment

tlabaj left a comment

patternfly-build commented Apr 6, 2023

fix(icons): removing inline styles from SVG icons #5275

fix(icons): removing inline styles from SVG icons #5275

Conversation

wojta commented Dec 17, 2020 • edited

wojta Dec 17, 2020

Choose a reason for hiding this comment

redallen Jan 4, 2021 • edited

Choose a reason for hiding this comment

mcoker Jan 4, 2021

Choose a reason for hiding this comment

wojta Jan 6, 2021 • edited

Choose a reason for hiding this comment

patternfly-build commented Dec 17, 2020 • edited

codecov-io commented Jan 13, 2021 • edited

Codecov Report

mcoker left a comment

Choose a reason for hiding this comment

wojta commented Jan 14, 2021 • edited

tlabaj commented Jan 19, 2021

wojta commented Jan 19, 2021

tlabaj commented Feb 3, 2021

mcoker commented Feb 8, 2021

redallen left a comment

Choose a reason for hiding this comment

redallen left a comment

Choose a reason for hiding this comment

nicolethoen commented Mar 29, 2023

jenny-s51 left a comment

Choose a reason for hiding this comment

srambach Mar 31, 2023

Choose a reason for hiding this comment

jenny-s51 Mar 31, 2023 • edited

Choose a reason for hiding this comment

srambach Mar 31, 2023

Choose a reason for hiding this comment

srambach Mar 31, 2023

Choose a reason for hiding this comment

mcoker left a comment

Choose a reason for hiding this comment

tlabaj left a comment

Choose a reason for hiding this comment

jenny-s51 left a comment

Choose a reason for hiding this comment

tlabaj left a comment

Choose a reason for hiding this comment

patternfly-build commented Apr 6, 2023

wojta commented Dec 17, 2020 •

edited

redallen Jan 4, 2021 •

edited

wojta Jan 6, 2021 •

edited

patternfly-build commented Dec 17, 2020 •

edited

codecov-io commented Jan 13, 2021 •

edited

wojta commented Jan 14, 2021 •

edited

jenny-s51 Mar 31, 2023 •

edited