RSP-1127|RSP-1128: ProgressCircle and ProgressBar #9
Conversation
pst67662
requested review from
dannify,
devongovett and
snowystinger
as code owners
|
Build successful! View the storybook |
|
Build successful! View the storybook |
|
Build successful! View the storybook |
|
Build successful! View the storybook |
pst67662
changed the title
|
Build successful! View the storybook |
|
Build successful! View the storybook |
pst67662
changed the title
packages/@adobe/spectrum-css-temp/components/barloader/index.css
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| .spectrum-CircleLoader.react-spectrum-Wait--centered { |
There was a problem hiding this comment.
I don't know if we want to support this type of centering anymore? @dannify ?
There was a problem hiding this comment.
Yes, we do (any reason we don't @snowystinger?). But I'm not sure if spectrum-css wants that feature. Last time they didn't, so we just put it in RSP. We could probably leave this css in RSP if that remains the case. cc. @lazd @GarthDB
There was a problem hiding this comment.
hmm... i don't know where my previous response went, but i'll re-write it best as i can remember
I'm thinking this kind of centering is less needed now thanks to flex box. It also causes problems because people forget that an absolute positioned item is removed from document flow, so the parent could easily end up with height 0. It can also end up on top of other elements because it's not in the document flow.
|
Build successful! View the storybook |
|
Build successful! View the storybook |
|
Build successful! View the storybook |
|
|
||
| it.each` | ||
| Name | Component | ||
| ${'ProgressCircle'} | ${ProgressCircle} |
There was a problem hiding this comment.
do none of the rest of these tests share any common ground with V2?
| } | ||
| ref={ref} | ||
| {...ariaProps} | ||
| {...filterDOMProps(otherProps)}> |
There was a problem hiding this comment.
put filterDOMProps first in the element, that way you don't risk it overriding any of the props you've set
then do the spread of aria
then do any you're explicitly setting
| let ariaValue = ariaProps['aria-valuenow']; | ||
| if (ariaValue > 0 && ariaValue <= 50) { | ||
| angle = -180 + (ariaValue / 50 * 180); | ||
| subMask1Style.transform = 'rotate(' + angle + 'deg)'; |
There was a problem hiding this comment.
we prefer string literals when possible
`rotate(${angle}deg)`
| ref={ref} | ||
| dir={direction} | ||
| {...ariaProps} | ||
| {...filterDOMProps(otherProps)}> |
There was a problem hiding this comment.
change order of spreads to match guidelines written in other comment
|
|
||
| return { | ||
| ariaProps: { | ||
| id: labelledComponentAriaProps.id, |
There was a problem hiding this comment.
can we just spread labelledComponentAriaProps into this object?
| }, | ||
| labelAriaProps, | ||
| formattedValueLabel, | ||
| percentage |
There was a problem hiding this comment.
percentage isn't an attribute, please put it into an appropriate object
nor is formattedValueLabel
| @@ -0,0 +1,3 @@ | |||
| export function clamp(value: number, min: number = -Infinity, max: number = Infinity): number { | |||
| return Math.min(Math.max(+value, min), max); | |||
There was a problem hiding this comment.
if value is a number, then we don't need to coerce it here, lets do that before we call clamp
| ) | ||
| } | ||
| ref={ref} | ||
| dir={direction} |
There was a problem hiding this comment.
this should already be set on some much higher up element, why are we setting it here?
|
Build successful! View the storybook |
| expect(progressCircle).toHaveAttribute('aria-valuemax', '100'); | ||
| }); | ||
|
|
||
| it('handles submask defaults', () => { |
There was a problem hiding this comment.
do these tests not share the same qualities in v2?
There was a problem hiding this comment.
I'll change tests to do checks on both components
There was a problem hiding this comment.
@snowystinger I need some help with this one... these tests should cover the same style transform property from V2 https://github.com/adobe/react-spectrum/blob/master/test/Wait/Wait.js#L47
except in V2 child nodes are fetched by class names... is there other way to get child nodes in V3 except adding "test-dataid" property, so I can test it on both components?
There was a problem hiding this comment.
o, hmmm... that is unfortunate, we actually can't do dataid tests for v2 either, they'll get stripped out in the build
I think we'll just have to leave v2 out of this one
thanks for looking into it
There was a problem hiding this comment.
add a comment to the file with that info though so we don't try to come along and add it later
|
Build successful! View the storybook |
|
Build successful! View the storybook |
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
Couple other things I notice in a rereview.
| @@ -0,0 +1,6 @@ | |||
| .spectrum-CircleLoader.react-spectrum-ProgressCircle--centered { | |||
There was a problem hiding this comment.
I think you'll have to get rid of the .spectrum-CircleLoader part if you want this css to be applied to the ProgressCircle component correctly
| } = props; | ||
|
|
||
| value = clamp(value, min, max); | ||
| let percentage = value / (max - min); |
There was a problem hiding this comment.
Hrm, I should've caught this earlier and maybe I'm wrong but I think the percentage calculation should actually be as follows: (value - min) / (max - min). The current calculation fails on min value that aren't 0.
e.g.
min = -5
max = 5
value = 0
old percentage calc = 0 / (5 - (-5)) = 0/10 = 0%
actual percentage calc = (0 - (-5))/(5 - (-5)) = (5)/(10) = .5 = 50%
Might be worthwhile writing a test to cover this case since this looks like it was broken in v2 as well.
|
Build successful! View the storybook |
devongovett
left a comment
devongovett
left a comment
There was a problem hiding this comment.
Looks great! 🎉
One small name change. Also merged in master and got a few failures due to the new package linting. You'll need to update dependencies to point to the alpha versions we published, and fix those lint errors.
|
also noticed that the centered story is not centered 😆 |
|
Build successful! View the storybook |
|
Build successful! View the storybook |
|
@devongovett centered story fixed |
|
Build successful! View the storybook |
Closes
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Team: