Esri / calcite-design-system

A monorepo containing the packages for Esri's Calcite Design System
https://developers.arcgis.com/calcite-design-system/
Other
285 stars 76 forks source link

Refactor(Action Bar): match Figma and utilize Tailwind #2452

Closed julio8a closed 2 years ago

julio8a commented 3 years ago

Description This component needs to match Figma designs.

Acceptance Criteria The component appears as it does in Figma.

Related prs:

5/19 QA: the only additional changes needed: - upping margin-right for .button--text-visible .icon-container from mr-2 to mr-3: https://github.com/Esri/calcite-components/blob/master/src/components/calcite-action/calcite-action.scss#L110 - ensure font-size matches Figma (may need h suffix)

7/13 remaining items:

Design:

asangma commented 3 years ago

ActionBar doesn't support scale.

I believe the extra space around the trigger was addressed here.

There is white space around the action-menu items on hover. This was due to accessibility hover/focus states.

Is this a suggestion for a code change or a Figma update?

bstifle commented 3 years ago

Why would we need to do that with the hover state, to account for the focus ring...?

The hover/press states should go edge to edge

julio8a commented 3 years ago

Same comment I posted in the action-pad, currently, action-bar doesn't support scale, which we need to add (S and L). It's really odd how it works now where you can change the scale of action inside action bar and it breaks.

The goal would be to set it at the parent level (action-bar) and action would follow, in this instance.

macandcheese commented 3 years ago

Is there a way to address this at the action level? Why does the focus ring rely on space around the element?

This 1px space occurs when using calcite-action elements that are slotted (in this case, in a panel). Focus, hover, placement all seem to have slightly different visuals, and looks sloppy. Can we eliminate these small px differences with action?

Screen Shot 2021-08-11 at 9 46 37 AM Screen Shot 2021-08-11 at 9 49 26 AM Screen Shot 2021-08-11 at 9 46 21 AM Screen Shot 2021-08-11 at 9 49 15 AM
bstifle commented 3 years ago

agreed the margin is strange. things should be edge to edge. focus should just be an internal 2px stroke

asangma commented 3 years ago

In ActionMenu, the button inside Action is not actually getting focused. That's why the additional fun tims. The Action is getting active set on it, and the focus styles are targeted to slotted Actions with that prop.

@driskull Is there a straight-forward way for ActionMenu to tell its slotted Actions to handle focus (i.e. pass focus on to the button in Action's shadow DOM)?

driskull commented 3 years ago

@asangma unfortunately, no. The actions should not actually be focused so we need to have active styles that mimmic it better.

We're going to run into this more and more as we only allow focus on one element at a time for a component. Especially if the component opens a menu or something like that.

https://www.digitala11y.com/aria-activedescendant-properties/ https://www.w3.org/TR/wai-aria-practices-1.1/examples/menu-button/menu-button-actions-active-descendant.html

driskull commented 3 years ago

Here's a good summary: https://developers.google.com/web/fundamentals/accessibility/focus/using-tabindex#managing_focus_in_components

asangma commented 3 years ago

Thanks @driskull. @bstifle @macandcheese Does this help explain the situation and the reasoning behind the solution that was devised?

julio8a commented 3 years ago

@driskull, is this one considered installed too based on https://github.com/Esri/calcite-components/pull/2934?

driskull commented 3 years ago

I think so. no.

this is unrelated

jcfranco commented 2 years ago

@macandcheese Does this help explain the situation and the reasoning behind the solution that was devised?

If there's pending work regarding this, can we create a separate issue and close this one? From what I gather, the rest from the original issue has been addressed. Let me know if this isn't the case. cc @benelan

macandcheese commented 2 years ago

Yeah I don’t think this is an issue anymore at least for my case above.

jcfranco commented 2 years ago

Closing and marking as self-verified based on ☝️.