feat(Spinner): remove legacy variant and svg prop #8616
Conversation
|
Preview: https://patternfly-react-pr-8616.surge.sh A11y report: https://patternfly-react-pr-8616-a11y.surge.sh |
There was a problem hiding this comment.
Nice @gitdallas ! Default svg spinners are looking great. Left a couple of comments.
| ) : ( | ||
| <Spinner diameter="1em" isSVG aria-label={defaultProgressArialabel} /> | ||
| ); | ||
| const _progressIcon = progressIcon ?? <Spinner diameter="1em" aria-label={defaultProgressArialabel} />; |
There was a problem hiding this comment.
We have a prop for this now! 🥳 It's beta, but the icon component is beta, too. IMO we could promote isInline any time, it's really simple, so it should be promoted before or with <Icon>
| const _progressIcon = progressIcon ?? <Spinner diameter="1em" aria-label={defaultProgressArialabel} />; | |
| const _progressIcon = progressIcon ?? <Spinner isInline aria-label={defaultProgressArialabel} />; |
If we make this update, can you also update this comment? We could replace "1em" with "inline", or just remove that and say "Defaults to a spinner."
And if it's not scope creep, could we poke around and see if this is used anywhere else? I looked in the react-core dir and only found this one. I can open a separate issue if you'd prefer.
There was a problem hiding this comment.
@mcoker - looks like this was merged soon after your comments... lmk if you open a new issue
There was a problem hiding this comment.
@nicolethoen @tlabaj do you see any reason to hold off on using this beta prop (<Spinner isInline />)? Or maybe just go ahead and promote the prop?
7 participants
What: Closes #8349