Conversation
src/Apps/Order2/Routes/Checkout/Components/DeliveryOptionsStep/Order2DeliveryOptionsForm.tsx
Outdated
Show resolved
Hide resolved
| open={selectedPaymentMethod === "stripe-card"} | ||
| data-testid="stripe-card-collapse" | ||
| > | ||
| <Box hidden={!isStripeCardSelected} data-testid="stripe-card-collapse"> |
There was a problem hiding this comment.
Replaced Collaspe with <Box hidden as we are still able to tab items inside a closed Collaspe
There was a problem hiding this comment.
Is there an animation we are losing here or is collapse literally just a disappearing but not properly hidden box?
There was a problem hiding this comment.
I'm not seeing any differences in the animation.
erikdstock
left a comment
There was a problem hiding this comment.
Looks pretty comprehensive. A few minor questions!
| <RadioGroup | ||
| flexDirection="column" | ||
| defaultValue={defaultOption} | ||
| onSelect={option => { | ||
| setSelectedOption(option) | ||
| setFieldValue("deliveryOption", option) | ||
| checkoutTracking.clickedSelectShippingOption(option.type) | ||
| }} | ||
| > |
There was a problem hiding this comment.
👍 when i was doing the radio change i wondered about some components using this and some not. Are radios always supposed to be wrapped instead of just using onClicks in the radios themselves?
There was a problem hiding this comment.
Yes, we should always be wrapping multiple radios in RadioGroup.
src/Apps/Order2/Routes/Checkout/Components/OfferStep/Components/Order2OfferOptions.tsx
Show resolved
Hide resolved
| expect(screen.getByTestId("stripe-card-collapse")).toHaveStyle({ | ||
| height: "0px", | ||
| }) | ||
| expect(screen.getByTestId("stripe-card-collapse")).not.toBeVisible() |
| open={selectedPaymentMethod === "stripe-card"} | ||
| data-testid="stripe-card-collapse" | ||
| > | ||
| <Box hidden={!isStripeCardSelected} data-testid="stripe-card-collapse"> |
There was a problem hiding this comment.
Is there an animation we are losing here or is collapse literally just a disappearing but not properly hidden box?
| you authorize Artsy to save the bank account specified above.`} | ||
| > | ||
| <Clickable ml={0.5} style={{ lineHeight: 0 }}> | ||
| <Clickable aria-label="Direct debit authorization information" ml={0.5} style={{ lineHeight: 0 }}> |
There was a problem hiding this comment.
In other areas with this icon on my recent radio groups PR I was able to remove the lineHeight style with no bad effect that I could see. Different context inside the payment step perhaps, or maybe i was missing something.
| as="button" | ||
| type="button" |
There was a problem hiding this comment.
if this is marked as a button, does it need a clear label or aria-label or aria-labeledby?
| const [radioChild, ...otherChildren] = React.Children.toArray(children) | ||
| const clonedRadio = React.cloneElement(radioChild as React.ReactElement, { | ||
| onSelect, | ||
| selected, | ||
| }) | ||
|
|
There was a problem hiding this comment.
Interesting. What is going on here? What happened to our radioChild that we had to replace it with a clone 🦎 👶 ? And why are we throwing _value away? If it's not needed, can we remove the prop?
There was a problem hiding this comment.
RadioGroup in Palette manages selection state, but it's expecting Radio as a child, not RadioOptionRow. As for value, this is needed for RadioGroup selection tracking. Basically, by adding RadioOptionRow we are breaking how RadioGroup is intended to be used.
Another option would be to add the RadioOptionRow styles as an option to the component in Palette.
There was a problem hiding this comment.
Or we directly add the styles to Radio and we lose the reusability factor but it seems like a cleaner solution.
There was a problem hiding this comment.
@erikdstock so I ended up removing RadioOptionRow entirely as it was interfering with the tabing selection. If the grey background on select for radio groups becomes a standard thing across the app we can add it in Palette. Open to other ideas though.
#7061 Bundle Size — 8.65MiB (-0.02%).cd6eeef(current) vs 3b3202e main#7052(baseline) Warning Bundle contains 31 duplicate packages – View duplicate packages Bundle metrics
Bundle size by type
Bundle analysis report Branch rquartararo/order2-accessability Project dashboard Generated by RelativeCI Documentation Report issue |
The type of this PR is: chore
This PR solves EMI-3025
@artsy/emerald-devs
Description