feat(downstate): docs + implementation for example components#2520
Conversation
mdt2
changed the title
File metricsSummaryTotal size: 3.93 MB*
Detailsactionbutton
actiongroup
button
buttongroup
checkbox
closebutton
dial
fieldlabel
logicbutton
modal
page
picker
progresscircle
slider
splitview
table
tokens
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
|
🚀 Deployed on https://pr-2520--spectrum-css.netlify.app |
|
I don't necessarily thing this is something that should be addressed but I did notice on Safari and Windows(via assistive labs) that the checkbox active state squares out the checkbox borders while other browsers do not. Safari Active
|
|
should the down state have a |
| @@ -0,0 +1,82 @@ | |||
| name: Down state | |||
There was a problem hiding this comment.
We'll need to add the max-perspective token setup here once it's available, but design hasn't defined the token yet and therefore it's not in the token system at this time.
There isn't anything for this in the design docs so I reached out to PJ on Slack to ask 👍 |
pfulton
force-pushed
the
spectrum-two
branch
from
378633a to
98335ab
Compare
mdt2
force-pushed
the
mdt2/css-612-down-state
branch
from
c9b938f to
436d3f6
Compare
|
|
||
| &:active { | ||
| /* stylelint-disable-next-line spectrum-tools/no-unknown-custom-properties */ | ||
| transform: perspective(max(var(--spectrum-downstate-height), var(--spectrum-downstate-width) * var(--spectrum-component-size-width-ratio-down))) translateZ(var(--spectrum-component-size-difference-down)); |
There was a problem hiding this comment.
If we're expecting users to dynamically fetch this width/height data, will we need to namespace these to the component?
There was a problem hiding this comment.
Any use cases you have in mind where a user will do it from their end?
There was a problem hiding this comment.
Originally when we were exploring a class-based approach, users were be expected to set these width and height variables in their implementation. But if we go with the component-based approach shown in this PR, we may be able to set these and ship them as part of the CSS instead.
There was a problem hiding this comment.
Namespacing might be necessary if there are cases where a component with a down state is nested within another component that has a down state? I'm not sure if this is a possibility.
One possible route would be to use --mod naming on these? --mod-button-downstate-height?
pfulton
force-pushed
the
spectrum-two
branch
from
98335ab to
fec70b3
Compare
mdt2
force-pushed
the
mdt2/css-612-down-state
branch
2 times, most recently
from
aa7165e to
42629dc
Compare
|
Great start!! A couple of questions:
|
|
@Rajdeepc Good questions!
I agree, we should do some testing on mobile as well once the team agrees on the approach. Let me know if this answers your questions 🙂 |
After talking with Damian and PJ in Slack, this down state motion isn't jarring or problematic from an accessibility perspective, so we don't need a reduced motion alternative. |
mdt2
force-pushed
the
mdt2/css-612-down-state
branch
from
ba2e79f to
42629dc
Compare
| export const DownStateDocs = CustomButton.bind({}); | ||
| DownStateDocs.args = { | ||
| variant: "accent", | ||
| customStyles: { | ||
| "--spectrum-downstate-width": "72px", | ||
| "--spectrum-downstate-height": "32px" | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a WIP and I don't love the direction it's going so far...if the team agrees on this approach (to setup down state in each component's index.css), then we should do this in the template.js, and make it compatible with changing t-shirt sizing and platform scale. But this was the first step to just see it working on the new Foundations/Down state page.
Checkbox doesn't need this because it uses the component-size-minimum-perspective-down token, which is already defined. But for components that are larger than 24px, we have to calculate the perspective based on width and height. We've thought from the beginning that consumers would have to create their own implementation for setting the values of those width and height variables, and in button's case I think that's still true, because we don't statically set a width and height in the CSS...
| <Story of={CheckboxStories.Default} /> | ||
| <Story of={ButtonStories.DownStateDocs} /> |
There was a problem hiding this comment.
There's an outstanding issue with these where the global styles don't load unless you first go to a component page, so the buttons look wonky. Not sure yet if that will be a fix in this PR or a separate, more global one
There was a problem hiding this comment.
Was this a known issue before or something you discovered for the first time adding this story? I noticed this when creating a new story for the Checkboxes and Action Buttons in Foundations for corner-rounding as well.
There was a problem hiding this comment.
I think it has to do with the existing setup and where we're importing global tokens (probably in preview.js). It can probably be its own Jira card to work on.
There was a problem hiding this comment.
I don't see a problem with defining these in each component. I think it makes sense to keep it simple like this, with it being a single line of CSS, that may need to be slightly customized per component.
But if we go with the component-based approach shown in this PR, we may be able to set these and ship them as part of the CSS instead.
For the max formula, you mentioned that maybe we could set these for some components. What if we had fallback default values for the components, for when the width and height variables aren't set? Button for example might use the defined minimum component height as another fallback on downstate height. This could vary per component. Some might be able to be set directly if the component has a fixed size. Just a thought. It may not be necessary if we are expecting the consumer to always set the width and height variables.
I think you're on the right track with the docs page, and would just make some minor formatting adjustments noted below.
|
|
||
| &:active { | ||
| /* stylelint-disable-next-line spectrum-tools/no-unknown-custom-properties */ | ||
| transform: perspective(max(var(--spectrum-downstate-height), var(--spectrum-downstate-width) * var(--spectrum-component-size-width-ratio-down))) translateZ(var(--spectrum-component-size-difference-down)); |
There was a problem hiding this comment.
Namespacing might be necessary if there are cases where a component with a down state is nested within another component that has a down state? I'm not sure if this is a possibility.
One possible route would be to use --mod naming on these? --mod-button-downstate-height?
Sure thanks. This make sense. Also curious to know how are you shipping javascript? We haven't had a chance to discuss this but if you had any discussion do let me know. Since we are building a logic to tackle this from SWC side we would love to see an example. |
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
| } | ||
| } | ||
|
|
||
| &:not(.is-readOnly):active { |
There was a problem hiding this comment.
This approach will not work in case the SWC wants to disable downstate for checkbox. This is tightly coupled with the css transform. We need to cascade the --spectrum-downstate-width and --spectrum-downstate-height to this css.
There was a problem hiding this comment.
Hey Rajdeep! Can you help me understand what needs to change? One piece of context that might be helpful is for components like Checkbox which are smaller than 24px, the transform perspective is not based on the width and height of the component, it is based on a set value per the design spec.
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
* feat(downstate): docs + implementation for example components * docs: update mdx * docs: reorg, stories live within foundations * docs: decorator for down state dimension tokens * docs: fix mdx hierarchy console error * fix: small iconOnly button gets min perspective * docs: use markdown, update language * fix: disabled, readonly checkbox doesnt have down state * chore(button,checkbox): update package versions
7 participants
Description
This one was a bit of a doozy in that I tried a lot of different approaches. TLDR, we landed on down state needing to be implemented in each component that requires it in S2. The short story for that is that each component is slightly different, and an overall custom property for the transform value wasn't an option because custom props have to be defined before they're used (which doesn't play nicely with our dynamic width and height).
More details about what led to this approach
I started with looking at a purely class based implementation of down state and ran into complications. For components like Button where the whole component is interactable, class based works fine because you can apply the class to the root/parent of the component. But with a more complex component like Checkbox, you have to apply the styles to the box itself, but the `active` state that we're targeting happens at the root/parent level.So then I experimented with this mix of utility class and placeholder approaches, but it felt a bit ick to keep both because it could lead to confusion for users. We also ran into issues with a placeholder approach when working on tooltip after which we landed on not using the placeholders at all. At this point I'm thinking that users shouldn't really need utility classes since this feature should be shipped with the components that need it anyway, and placeholders that rely on custom props haven't worked well in the past.
Then Josh suggested that I try creating a custom token for the transform value, to keep the code DRY. Custom properties have to be defined before they're used, so I had to define
--spectrum-downstate-widthand--spectrum-downstate-heightas0pxeach incustom-vars.cssfor the transform value (perspective(max(var(--spectrum-downstate-height), var(--spectrum-downstate-width) * var(--spectrum-component-size-width-ratio-down))) translateZ(var(--spectrum-component-size-difference-down));) to be considered defined by the browser. The problem is that even when I was updating those width/height custom props using thestyletag, the global custom token wasn't picking up those new values, it would always use 0px. So this option was also, unfortunately, ruled out.And that brings us to the solution detailed in the most recent commit of this PR, which is to implement this transform within the components that need the feature in S2. Button and checkbox are the ones I included in this work to demo because Lynn asked to see those as examples in our Storybook (she also asked us to show Select Box since it's like 200x200px, but this is an S2 component we haven't built yet so I'm waiting to hear back from Lynn on if there is a large size S1 component we can use as an example in the meantime). As we pick up cards to migrate components to S2, adding down state to those that require it should be included in the work on those cards. The documentation provided in the docs site as part of this PR is intended to help guide that work.
How and where has this been tested?
Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.
Validation steps @jawinn
To-do list