-
Notifications
You must be signed in to change notification settings - Fork 195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(downstate): docs + implementation for example components #2520
Conversation
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 |
should the down state have a |
@@ -0,0 +1,82 @@ | |||
name: Down state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍 |
378633a
to
98335ab
Compare
c9b938f
to
436d3f6
Compare
components/button/index.css
Outdated
@@ -147,6 +147,11 @@ governing permissions and limitations under the License. | |||
box-shadow: none; | |||
} | |||
|
|||
&: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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any use cases you have in mind where a user will do it from their end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
98335ab
to
fec70b3
Compare
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. |
ba2e79f
to
42629dc
Compare
DownStateDocs.args = { | ||
variant: "accent", | ||
customStyles: { | ||
"--spectrum-downstate-width": "72px", | ||
"--spectrum-downstate-height": "32px" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
components/button/index.css
Outdated
@@ -147,6 +147,11 @@ governing permissions and limitations under the License. | |||
box-shadow: none; | |||
} | |||
|
|||
&: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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
* 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
* 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
@@ -150,6 +150,12 @@ governing permissions and limitations under the License. | |||
} | |||
} | |||
|
|||
&:not(.is-readOnly):active { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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-width
and--spectrum-downstate-height
as0px
each incustom-vars.css
for 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 thestyle
tag, 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