[SvgIcon] Allow viewBox to inherit from Component through inheritViewBox prop #29954
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alex-dikusar
changed the title
alex-dikusar
force-pushed
the
svgicon/pass-viewbox-from-custom-component
branch
from
288f435 to
90cedcc
Compare
mnajdova
reviewed
Dec 1, 2021
|
@alex-dikusar are you planning to finish this PR anytime soon? :) |
alex-dikusar
force-pushed
the
svgicon/pass-viewbox-from-custom-component
branch
from
d483e37 to
453769b
Compare
1 task
michaldudak
approved these changes
Dec 30, 2021
There was a problem hiding this comment.
Looks OK to me. Thanks for your contribution, @alex-dikusar!
oliviertassinari
added
component: SvgIcon
There was a problem hiding this comment.
In https://mui.com/components/icons/#component-prop I believe we can replace viewBox="0 0 600 476.6" for this new prop. This seems important to update.
| @@ -96,4 +96,24 @@ describe('<SvgIcon />', () => { | |||
| expect(container.firstChild).to.have.class(classes.fontSizeInherit); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('prop: inheritViewBox', () => { | |||
| const customSvg = (props) => ( | |||
There was a problem hiding this comment.
This is a component, it's should be uppercased.
Suggested change
| const customSvg = (props) => ( | |
| const CustomSvg = (props) => ( |
Comment on lines
71
to
+72
| titleAccess, | ||
| inheritViewBox = false, |
There was a problem hiding this comment.
To keep it sorted ASC
Suggested change
| titleAccess, | |
| inheritViewBox = false, | |
| inheritViewBox = false, | |
| titleAccess, |
Comment on lines
+52
to
+56
| * Useful when you want to reference a custom `component` and have `SvgIcon` pass that | ||
| * `component`'s viewBox to the root node. | ||
| * If `true`, the root node will inherit the custom `component`'s viewBox and the `viewBox` | ||
| * prop will be ignored. | ||
| * @default false |
There was a problem hiding this comment.
In the other props description, I believe we first describe what the prop does, and only after why it might be used for:
Suggested change
| * Useful when you want to reference a custom `component` and have `SvgIcon` pass that | |
| * `component`'s viewBox to the root node. | |
| * If `true`, the root node will inherit the custom `component`'s viewBox and the `viewBox` | |
| * prop will be ignored. | |
| * @default false | |
| * If `true`, the root node will inherit the custom `component`'s viewBox and the `viewBox` | |
| * prop will be ignored. | |
| * Useful when you want to reference a custom `component` and have `SvgIcon` pass that | |
| * `component`'s viewBox to the root node. | |
| * @default false |
| @@ -97,7 +97,7 @@ If you need a custom SVG icon (not available in the [Material Icons](/components | |||
| This component extends the native `<svg>` element: | |||
|
|
|||
| - It comes with built-in accessibility. | |||
| - SVG elements should be scaled for a 24x24px viewport so that the resulting icon can be used as is, or included as a child for other MUI components that use icons. (This can be customized with the `viewBox` attribute). | |||
| - SVG elements should be scaled for a 24x24px viewport so that the resulting icon can be used as is, or included as a child for other MUI components that use icons. (This can be customized with the `viewBox` attribute, to inherit `viewBox` value from original image `inheritViewBox` attribute can be used). | |||
There was a problem hiding this comment.
At this point, the parenthesis probably is not relevant.
Suggested change
| - SVG elements should be scaled for a 24x24px viewport so that the resulting icon can be used as is, or included as a child for other MUI components that use icons. (This can be customized with the `viewBox` attribute, to inherit `viewBox` value from original image `inheritViewBox` attribute can be used). | |
| - SVG elements should be scaled for a 24x24px viewport so that the resulting icon can be used as-is, or included as a child for other MUI components that use icons. This can be customized with the `viewBox` attribute, to inherit `viewBox` value from the original image `inheritViewBox` attribute can be used. |
wladimirguerra
pushed a commit
to wladimirguerra/material-ui
that referenced
this pull request
Feb 2, 2022
…Box prop (mui#29954) Co-authored-by: Gonçalo Vieira Figueiredo <me@goncalovf.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #18782
Based on previous PR dedicated to that issue, just fixed last unresolved comments and opened my own PR on it.