fix: adding max height and full height to picker menu component #3051
Conversation
🦋 Changeset detectedLatest commit: 1e39eff The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
🚀 Deployed on https://pr-3051--spectrum-css.netlify.app |
File metricsSummaryTotal size: 4.10 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailsalertdialog
assetlist
avatar
button
calendar
card
closebutton
dialog
fieldlabel
modal
picker
popover
site
stepper
toast
tooltip
treeview
underlay
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
|
PR Looking great! Just an FYI. This issue was also being reported while using Tray component in isolation. So would it also be okay to add this property in tray.css too so that users can have control on the max-height? |
| @@ -136,13 +138,10 @@ export const Template = ({ | |||
| isDisabled, | |||
| isReadOnly, | |||
| customClasses, | |||
| customStyles: { | |||
There was a problem hiding this comment.
@rubencarvalho I removed the inline styling of the Picker because it was displaying the label and button inline and overriding the flex-box styling and disrupting the text wrap especially when the label was displayed left. So I "turned it off" by removing this block. I can bring it back if needed
There was a problem hiding this comment.
Oh, thanks for the context! I thought it was a forgotten edit 😄
components/picker/index.css
Outdated
| /* Fixed menu height */ | ||
| .spectrum-Picker--menu-height-fixed { | ||
| & + .spectrum-Popover.is-open { | ||
| max-block-size: var(--mod-picker-menu-height-fixed, 200px); |
There was a problem hiding this comment.
Just for context, did we confirm this default value with design?
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@spectrum-css/picker": major | |||
There was a problem hiding this comment.
Should this be a major bump? Seems like this feature is non-breaking.
| @@ -39,6 +40,7 @@ export const Picker = ({ | |||
| ["is-open"]: isOpen, | |||
| ["is-loading"]: isLoading, | |||
| ["is-keyboardFocused"]: isKeyboardFocused, | |||
| [`${rootClass}--menu-height-fixed`]: menuHeight, | |||
There was a problem hiding this comment.
Have we considered the pros and cons of not having a dedicated API for this feature? For instance, instead of having a specific menu-height class, we could use a generic mod variable applied to the base class. This mod variable would function as a feature toggle, although customers would need to define its value themselves (because we would not have a default fallback).
There was a problem hiding this comment.
But I guess this is a more holistic API design question, maybe the CSS team can help understand when a mod is enough vs a dedicated toggleable class feature.
There was a problem hiding this comment.
I agree, this feels like a good use-case to add a mod rather than new API.
| @@ -52,6 +52,16 @@ export default { | |||
| }, | |||
| isQuiet, | |||
| isOpen, | |||
| menuHeight: { | |||
| name: "Max Menu Height", | |||
There was a problem hiding this comment.
I would recommend renaming it to "Menu max block size" (doesn't sound as nice, but I would be for accuracy) - what do you think?
|
|
||
| /* Fixed menu height */ | ||
| .spectrum-Picker--menu-height-fixed { | ||
| & + .spectrum-Popover.is-open { |
There was a problem hiding this comment.
Do we need the .is-open here? Can't the popover max-block-size be defined independently of being open?
There was a problem hiding this comment.
For specificity, I think it's necessary. It also gives other devs more context.
There was a problem hiding this comment.
Looking at the render function in the packages/picker/src/Picker.ts, I don't think this is a "Picker" feature so much as it's a "Menu" feature. If we add a block-size property to the root Menu class:
.spectrum-Menu {
display: inline-block;
inline-size: var(--mod-menu-inline-size, auto);
block-size: var(--mod-menu-block-size, auto);
}
Users now have a hook to set a 200px fixed height if they so wish. If the design guidance is to default to 200px, that's a breaking change so maybe we can make note of it to add in the future and deprecate a block-size of auto?
| @@ -124,6 +125,7 @@ | |||
| "--mod-picker-icon-color-key-focus", | |||
| "--mod-picker-inline-size", | |||
| "--mod-picker-line-height", | |||
| "--mod-picker-menu-height-fixed", | |||
There was a problem hiding this comment.
Same comment as the other one, around the naming 😛
| @@ -124,6 +125,7 @@ | |||
| "--mod-picker-icon-color-key-focus", | |||
| "--mod-picker-inline-size", | |||
| "--mod-picker-line-height", | |||
| "--mod-picker-menu-height-fixed", | |||
There was a problem hiding this comment.
For the sake of making our discussion public for other folks to be able to follow along, I suggest we consider adding a CSS property to the popover component that allows it to be styled externally. This approach would simplify our implementation on the web components side.
Here's a rough example of how this could look like:
/* In the popover component */
.spectrum-Popover {
max-block-size: var(--spectrum-popover-max-block-size);
}
/* In the picker component */
.spectrum-Picker--menu-height-fixed {
& + .spectrum-Popover.is-open {
--spectrum-popover-max-block-size: var(--mod-picker-menu-height-fixed, 200px);
}
}There was a problem hiding this comment.
max-block-size: var(--spectrum-popover-max-block-size);
This would require us defining a global variable that can be shared.
| @@ -124,6 +125,7 @@ | |||
| "--mod-picker-icon-color-key-focus", | |||
| "--mod-picker-inline-size", | |||
| "--mod-picker-line-height", | |||
| "--mod-picker-menu-height-fixed", | |||
There was a problem hiding this comment.
@castastrophe @pfulton
Just to confirm, should we continue leaning more into using the mod properties for features, or is there an alternative approach that would be preferred?
There was a problem hiding this comment.
For now, this is our approach. We may revisit it in the near future, but currently we will move forward with this approach (so it's valid here).
… to other components
…/adobe/spectrum-css into aramos-adobe/add-max-height-menu
|
@rubencarvalho Ready for review my man! |
pfulton
added
the
do not merge
|
@pfulton Would you let us know what has been decided here? I see its still open! |
|
|
||
| /* Fixed menu height */ | ||
| .spectrum-Picker--menu-height-fixed { | ||
| & + .spectrum-Popover.is-open { |
There was a problem hiding this comment.
Looking at the render function in the packages/picker/src/Picker.ts, I don't think this is a "Picker" feature so much as it's a "Menu" feature. If we add a block-size property to the root Menu class:
.spectrum-Menu {
display: inline-block;
inline-size: var(--mod-menu-inline-size, auto);
block-size: var(--mod-menu-block-size, auto);
}
Users now have a hook to set a 200px fixed height if they so wish. If the design guidance is to default to 200px, that's a breaking change so maybe we can make note of it to add in the future and deprecate a block-size of auto?
| @@ -39,6 +40,7 @@ export const Picker = ({ | |||
| ["is-open"]: isOpen, | |||
| ["is-loading"]: isLoading, | |||
| ["is-keyboardFocused"]: isKeyboardFocused, | |||
| [`${rootClass}--menu-height-fixed`]: menuHeight, | |||
There was a problem hiding this comment.
I agree, this feels like a good use-case to add a mod rather than new API.
Description
The picker menu list can get very extensive. Why don't we add the ability to set a fixed height to scroll or extend the menu to the height of the viewport where a user can select their option.
is-fixedwould be the attribute to enable the CSS modificationbooleanin Storybook to enable fixed height ifisOpenis trueHow 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
Regression testing
Validate:
Screenshots
To-do list