finos / a11y-theme-builder

DesignOps toolchain theme builder for accessibility inclusion using Atomic Design.
Apache License 2.0
41 stars 70 forks source link

[TB] Change Elevation and Bevel Dropdown selectors to Shadows #413

Closed lwnoble closed 11 months ago

lwnoble commented 1 year ago

Problem/Concern

Now that we have multiple shadow styles we should offer the styles in one Shadow dropdown menu

Proposed Solution

Shadow:
<Select>
None
**Elevations**
Elevation 1
Elevation 2
Elevation 3
Elevation 4
Elevation 5
Elevation 6
Elevation 7
Elevation 8
Elevation 9
**Bevels**
Bevel 1
Bevel 2
Bevel 3
Bevel 4
Bevel 5
Bevel 6
Bevel 7
Bevel 8
Bevel 9
**Inset Bevels**
Inset Bevel 1
Inset Bevel 2
Inset Bevel 3
Inset Bevel 4
Inset Bevel 5
Inset Bevel 6
Inset Bevel 7
Inset Bevel 8
Inset Bevel 9
**Grooves**
Groove 1
Groove 2
Groove 3
Groove 4
Groove 5
Groove 6
Groove 7
Groove 8
Groove 9
**Ridges**
Ridge 1
Ridge 2
Ridge3
Ridge 4
Ridge 5
Ridge 6
Ridge 7
Ridge 8
Ridge 9 
**Recesses**
Recess 1
Recess 2
Recess 3
Recess 4
Recess 5
Recess 6
Recess 7
Recess 8
Recess 9
**Glow**
Glow 1
Glow 2
Glow 3
Glow4
Glow 5
Glow 6
Glow 7
Glow 8
brycecurtis commented 1 year ago

@lwnoble Questions about this issue:

  1. Do these selector values go into a new Shadow atom?
  2. What css and JSON need to be generated from this atom?
lwnoble commented 1 year ago

@bryce - 1.) I am happy to collapse all the shadows and bevels into one atom. I will do that today. 2.) The css is already being generated in the code as is the JSON.

lwnoble commented 1 year ago

@bryce I have consolidated the shadow atom - all under the "Shadow Atoms" with a toggle

aaronreed708 commented 1 year ago

@brycecurtis to look at with eye toward #243

aaronreed708 commented 1 year ago

closing per Bryce as done.

aaronreed708 commented 1 year ago

Reopening this issue, I think it was closed in error. According to @lwnoble above, the dropdown menu that we should be seeing for all "shadow" type of molecule properties, should be:

Shadow
   None
Elevations
   Elevation 1
   Elevation 2
   Elevation 3
   Elevation 4
   Elevation 5
   Elevation 6
   Elevation 7
   Elevation 8
   Elevation 9
Bevels
   Bevel 1
   Bevel 2
   Bevel 3
   Bevel 4
   Bevel 5
   Bevel 6
   Bevel 7
   Bevel 8
   Bevel 9
Inset Bevels
   Inset Bevel 1
   Inset Bevel 2
   Inset Bevel 3
   Inset Bevel 4
   Inset Bevel 5
   Inset Bevel 6
   Inset Bevel 7
   Inset Bevel 8
   Inset Bevel 9
Grooves 
   Groove 1
   Groove 2
   Groove 3
   Groove 4
   Groove 5
   Groove 6
   Groove 7
   Groove 8
   Groove 9
Ridges
   Ridge 1
   Ridge 2
   Ridge3
   Ridge 4
   Ridge 5
   Ridge 6
   Ridge 7
   Ridge 8
   Ridge 9
Recesses
   Recess 1
   Recess 2
   Recess 3
   Recess 4
   Recess 5
   Recess 6
   Recess 7
   Recess 8
   Recess 9
 Glow
   Glow 1
   Glow 2
   Glow 3
   Glow 4
   Glow 5
   Glow 6
   Glow 7
   Glow 8

@smithbk unless I am missing something, I don't see any code in the SDK that is building a PropertyXxxxxSelectable with this kind of content. So I believe that this issue is meant to create such code in the SDK and for the Sliders.handleElevation and Sliders.barInsetShadow to reflect when one of those values is selected and generate the correct CSS var values.

aaronreed708 commented 1 year ago

I talked with @smithbk yesterday,

We are concerned about the user experience of having to scroll such a large menu. So I took the todo to investigate better ways to potentially structure the UI here rather than a long dropdown. We also came up with these questions that we need to answer:

  1. are all of these shadow styles appropriate for all molecules? E.g. on this slider issue specifically, does an Inverse Bevel 9 value make sense for Handle Elevation or does Elevation 9 make sense for Bar Inset Shadow?
  2. Will all of the CSS values possible from all of these CSS variables work with the CSS that consumes them? For example, var(--sliderhandleElevation) is used as a value for "box-shadow" in both Theme.jsx and Theme.css. Will all of the possible values for bevel, elevation, glow, etc. conform to what box-shadow expects?

I asked @lwnoble about #1. Her reply was, "They are not all common but I want to give designers freedom to create beyond the standard and all of those shadows are hard to figure out and make work in light and dark mode."

The plan is to talk on our Theme Builder call today on how to best move this issue forward.

evangk6 commented 1 year ago

Keith to update SDK to support above.

smithbk commented 1 year ago

@lwnoble @brycecurtis @aaronreed708 Two things to note as I'm doing this

1) The SDK doesn't currently generate any CSS variables for Grooves, Ridges, or Recesses, so I will need to know how to generate the CSS if one of these values are selected.

2) The "Shadow None" seems to indicate that there is no default value; however, if none of these fields have a default value, then we will be changing the functionality in that the CSS variables will not be generated from these fields until they are set, putting more on the user. I would recommend that we keep the same default values for all of these fields. OK?

aaronreed708 commented 1 year ago

Talked with @smithbk and came up with a structure for the data coming from the SDK. We both like https://mui.com/material-ui/react-select/#grouping as a good way to show the menu in the near term. I opened #551 to create the replacement component.

aaronreed708 commented 1 year ago

@lwnoble @brycecurtis @aaronreed708 Two things to note as I'm doing this

  1. The SDK doesn't currently generate any CSS variables for Grooves, Ridges, or Recesses, so I will need to know how to generate the CSS if one of these values are selected.

@smithbk it looks like the values for these CSS variables is currently kept here: https://github.com/discoverfinancial/a11y-theme-builder/blob/d4040b15a67a522af4e4ec90dfb09e7d8421a256/code/src/ui/src/mui-a11y-tb/themes/TB.css#L269. Ridge, Groove and Recessed

smithbk commented 1 year ago

@lwnoble @aaronreed708 @brycecurtis The labels that users are going to see for these fields along with the possible selections for those fields is going to seem weird to me. For example, consider the chart donut molecule which has 3 fields called "Segment Elevation", "Segment Bevel", and "Container Reverse Bevel". Consider that each of these fields will now have the same labels but each will have all of the selectable values listed in the description of this issue.

Questions:

  1. Should I be changing the labels associated with these fields?
  2. Is it possible that some of these fields values, if both are set, are really mutually exclusive? For example, can "Segment Elevation" and "Segment Bevel" both be active at the same time for the chart donut molecule?
lwnoble commented 1 year ago

New css variables

--button-shadow: --chip-shadow: --card-shadow: --tooltip-shadow: --modal-shadow: --dropdown-shadow: --toast-shadow: --image-shadow: --avatar-shadow: --popover-shadow:

smithbk commented 12 months ago

@lwnoble Changes have been checked into the "shadows" branch and waiting for corresponding changes to be made to the UI before merging.

aaronreed708 commented 11 months ago

Committed with Lise's other styling changes and Keith's sdk changes. Closing.