-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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: update svgr webpack config to use svg dimensions #24747
fix: update svgr webpack config to use svg dimensions #24747
Conversation
22913f6
to
d764f79
Compare
d764f79
to
184ffd1
Compare
/testenv up |
@eschutho Container image not yet published for this PR. Please try again when build is complete. |
@eschutho Ephemeral environment creation failed. Please check the Actions logs for details. |
/testenv up |
@eschutho Ephemeral environment spinning up at http://34.222.110.123:8080. Credentials are |
Codecov Report
@@ Coverage Diff @@
## master #24747 +/- ##
==========================================
- Coverage 68.89% 67.23% -1.66%
==========================================
Files 1901 1902 +1
Lines 73925 73939 +14
Branches 8183 8176 -7
==========================================
- Hits 50931 49716 -1215
- Misses 20873 22110 +1237
+ Partials 2121 2113 -8
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 97 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@eschutho do you see #24740? It seems @mistercrunch had an alternative formulation. |
@@ -419,11 +419,6 @@ const config = { | |||
{ | |||
loader: '@svgr/webpack', | |||
options: { | |||
icon: true, |
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.
are the defaults good? do we want to be explicit as in
options: {
// making sure that svgo isn't setting width:1em
icon: false,
// don't allow svgo to alter/opitmize the svg
svgo: false,
},
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.
Yeah, good idea. The default for svgo is actually true and I don't think it will break anything in this use case if changed to false, but I'll add in the default for icon with a comment.
Thanks @john-bodley, yeah I saw @mistercrunch's PR as well. This PR just reverts the functionality back to the original behavior where it does not override the width/height if it's in the svg file. @mistercrunch, @rusackas and I talked a bit offline about what the long-term behavior should be and agreed that we'll merge both PRs with the idea that if you wanted to use height/width in the svg that it should render in the html, or if you want to override the values with css, you can do that as well. |
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.
LGTM!
Ephemeral environment shutdown and build artifacts deleted. |
(cherry picked from commit f856ba2)
SUMMARY
This fix from a breaking change/regression after bumping the svgr/webpack package enables us to continue to use the width/height dimensions from the svg image itself. Ref: https://react-svgr.com/docs/options/#icon
Changes: https://github.com/apache/superset/pull/24648/files#diff-3834c36280e1af5685801f3357578aaec5a110699a2b8eb97844c56b4400c091
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
Load a chart with icons. There may be other places in the application that were affected that haven't yet been identified, but by doing a code search for svg images with width and height we should be able to find others.
ADDITIONAL INFORMATION