30 react-aria poc #34
Conversation
Initial attempt to replicate existing composite components with components from react-aria library.
| @@ -0,0 +1,61 @@ | |||
| @import "../styles/theme.css"; | |||
|
|
|||
| .react-aria-Toolbar { | |||
There was a problem hiding this comment.
Probably don't need react-aria in the class name.
There was a problem hiding this comment.
That's the default for the components in react-aria
| .react-aria-Toolbar { | ||
| display: flex; | ||
| flex-wrap: wrap; | ||
| gap: 5px; |
There was a problem hiding this comment.
I think that most, if not all of these hard-coded pixel values should either be set globally e.g., gap-small: 5px, inherited (maybe as a flag?), or a property in the component.
| border-radius: 4px; | ||
| appearance: none; | ||
| vertical-align: middle; | ||
| font-size: 1rem; |
There was a problem hiding this comment.
That's a very small font size. Again should be inherited.
| color: var(--text-color); | ||
| background: var(--button-background); | ||
| border: 1px solid var(--border-color); | ||
| border-radius: 4px; |
There was a problem hiding this comment.
Should be a global var, or a property. Some applications won't need rounded buttons.
There was a problem hiding this comment.
Right. Everything here so far has been taken straight from the react-aria example documentation. We'll need to put more thought into what is specific to each component vs what is applied across all components in the library.
We'll provide some level of customization through a design tokens file and css variables but we'll need to put some thought into the level of customization that will be required (it could potentially be very overwhelming so we'll need to strike a balance)
| font-size: 1rem; | ||
| text-align: center; | ||
| margin: 0; | ||
| outline: none; |
There was a problem hiding this comment.
Removing the outline can cause a11y issues.
There was a problem hiding this comment.
Good to know. Again, this is from an example in the docs but can be changed easily
There was a problem hiding this comment.
I think this removal is to resolve some janky issues that outlines can have with borders,.It looks like it's addressed in the following rules.
| outline: none; | ||
| padding: 6px 10px; | ||
| text-decoration: none; | ||
| min-width: 34.5px; |
There was a problem hiding this comment.
I'm not sure if decimal pixel values are supported in all browsers, or if they'll behave in a way that makes sense to devs. I would round for clarity.
| .react-aria-Switch { | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 0.571rem; |
There was a problem hiding this comment.
All from examples in the docs
| height: 0.857rem; | ||
| background: var(--highlight-background); | ||
| border-radius: 16px; | ||
| transition: all 200ms; |
There was a problem hiding this comment.
Maybe needs a prefers-reduced-motion media query.
There was a problem hiding this comment.
Would it essentially be the same, but with no transition?
| isSelected={isActive} | ||
| onChange={onChange} | ||
| > | ||
| <div className="indicator" /> |
There was a problem hiding this comment.
Self-closing div? Is this a react thing?
There was a problem hiding this comment.
Yeah I think so. The auto formatting changes this <div className="indicator"></div> to this <div className="indicator" />
| <div className="layerCard"> | ||
| <div className="layerCardWrapper"> | ||
| <Switch | ||
| isSelected={isActive} |
There was a problem hiding this comment.
isSelected? isActive? I think we need to stick with the HTML nomenclature. I don't know what this does just by looking at it. Is it enabled? i.e., disabled='false'?
I suggest using isDisabled since that is the HTML attribute (if that's what this does).
There was a problem hiding this comment.
This is from the react-aria library. It related to making it a "controlled component" in react terms.
The isSelected prop can be used to make the selected state controlled. The onChange event is fired when the user presses the switch, and receives the new value.
I think if we go with this library there are a few quirks like this. They're well documented but deviate from traditional HTML. We'll need to decide if it's worth the tradeoff.
There was a problem hiding this comment.
We can change the isActive prop to isSelected for consistency if we want. As @JBurkinshaw says we will have to catch and update a bunch of these inconsistencies when adopting the library.
Description
Initial attempt to replicate existing composite components with components from react-aria library. Covered everything except for the date range slider so far.
Not ready to merge. Opened a draft PR so others can take a look.
Related issue: #30
Type of change
Delete options that are not relevant.
Instructions
npm run storybookReactAria" suffix.How Has This Been Tested?