[Accordion] Update Accordion.Container-based Styling #371
Conversation
| @@ -42,6 +52,8 @@ export const AccordionBox = styled.div<{ | |||
| }>` | |||
| ${({ noBorder, isOpen }) => (!noBorder ? getBorderStyle(isOpen) : '')}; | |||
|
|
|||
| position: relative; | |||
There was a problem hiding this comment.
You can play with this in the dev tools but without it the box-shadow in the focus state is clipped by the border. It's not a total improvement but it's a small thing before I get to the accessibility updates this cycle, which is changing our focus state anyway.
docs/accordion.md
Outdated
| <Accordion.Grouping> | ||
| <Accordion.Container> | ||
| <Accordion title={<Accordion.Content>First Title</Accordion.Content>}> | ||
| <Accordion.Content>First Expansion</Accordion.Content> | ||
| </Accordion> | ||
| </Accordion.Container> | ||
| <Accordion.Container> | ||
| <Accordion title={<Accordion.Content>Second Title</Accordion.Content>}> | ||
| <Accordion.Content>Second Expansion</Accordion.Content> | ||
| </Accordion> | ||
| </Accordion.Container> | ||
| </Accordion.Grouping>; |
There was a problem hiding this comment.
This component structure confuses me a little bit. My understanding of Container was that it was an outer wrapper to hold several instances of Accordion. Here's an example from PocketDerm that supports that: https://github.com/curology/PocketDerm/blob/6216330178f842b1a886a23baf0b6a654aeb98f1/resources/assets/jsx/components/photoUploadAccordion/index.js#L160
With that in mind, I'm curious if it's feasible to extend the styles on Container with the Grouping styles and not have to use two outer wrappers for Accordion?
There was a problem hiding this comment.
Oh interesting. Since there were some usages of <Accordion> without the container I had assumed that it just wraps a single <Accordion>. I should have considered more strongly that the docs read Container to hold instances of <Accordion> even though all the code examples only had one <Accordion> present. Let me refactor.
There was a problem hiding this comment.
@ZeMunchkin Thank you again for catching this. Updated in 03af86c (and story markup formatting fixed in 7d95b2b). I've also updated the PR description.
| const getTopBorderRadius = (borderRadius: string) => ({ | ||
| borderTopLeftRadius: borderRadius, | ||
| borderTopRightRadius: borderRadius, | ||
| }); | ||
|
|
||
| const getBottomBorderRadius = (borderRadius: string) => ({ | ||
| borderBottomLeftRadius: borderRadius, | ||
| borderBottomRightRadius: borderRadius, | ||
| }); |
There was a problem hiding this comment.
I'm on the fence about these functions, since they seem to only give borderRadius two additional names without any modifications. It seems like it might be adding some unnecessary complexity, since borderRadius could be used in place of these to the same effect.
There was a problem hiding this comment.
Edit: misread this message the first time around. See follow-up, anyhow.
There was a problem hiding this comment.
@ZeMunchkin Opted to just remove the helpers entirely in ac26593 🙂
michaeljaltamirano
changed the title
|
Need to refactor the styling since the box-shadows are not applying accurately anymore in |
This PR sets out to make the following changes to the Accordion component:
box-shadowproperty.4pxborder-radius.The first two updates are relatively trivial. The third update required expanding the styling functionality of
Accordion.Container, which has additional CSS selectors that handle theborder-radius& focus outline requirements.border-radiusis set via long-hand propertiesborder-top-left-radius,border-top-right-radius,border-bottom-left-radius, andborder-bottom-right-radius.This PR also updates all documentation instances of
<Accordion>to be wrapped with<Accordion.Container>.You can play with the Review App here: https://curology-radiance-pr-371.herokuapp.com/
Before:
After: