Skip to content
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

fix: adding max height and full height to picker menu component #3051

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

aramos-adobe
Copy link
Collaborator

@aramos-adobe aramos-adobe commented Aug 29, 2024

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.

  • Added max-block-size capabilities
  • As a Web Component, is-fixed would be the attribute to enable the CSS modification
  • Added boolean in Storybook to enable fixed height if isOpen is true

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

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • VRTs have been run and looked at.
  • Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

To-do list

  • I have read the contribution guidelines.
  • I have updated relevant storybook stories and templates.
  • I have tested these changes in Windows High Contrast mode.
  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • ✨ This pull request is ready to merge. ✨

Copy link

changeset-bot bot commented Aug 29, 2024

🦋 Changeset detected

Latest commit: 1e39eff

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@spectrum-css/popover Minor
@spectrum-css/picker Major

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

@aramos-adobe aramos-adobe marked this pull request as draft August 29, 2024 15:39
Copy link
Contributor

github-actions bot commented Aug 29, 2024

🚀 Deployed on https://pr-3051--spectrum-css.netlify.app

Copy link
Contributor

github-actions bot commented Aug 29, 2024

File metrics

Summary

Total size: 4.10 MB*
Total change (Δ): ⬇ 2.61 KB (-0.06%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package Size Δ
alertdialog 5.14 KB ⬆ 0.03 KB
assetlist 7.91 KB ⬆ 0.01 KB
avatar 5.04 KB ⬆ 0.06 KB
button 83.00 KB ⬇ 0.99 KB
calendar 19.29 KB ⬆ 0.02 KB
card 20.64 KB ⬇ 0.02 KB
closebutton 15.69 KB ⬇ 0.14 KB
dialog 14.67 KB ⬆ 0.03 KB
fieldlabel 6.63 KB ⬆ 0.09 KB
modal 5.47 KB ⬆ 0.01 KB
picker 30.25 KB ⬆ 0.11 KB
popover 16.01 KB ⬆ 0.14 KB
site 14.26 KB ⬆ < 0.01 KB
stepper 20.54 KB ⬇ 0.11 KB
toast 8.60 KB ⬇ 0.03 KB
tooltip 33.97 KB ⬆ < 0.01 KB
treeview 17.06 KB ⬇ 0.10 KB
underlay 3.27 KB ⬆ 0.01 KB

Details

alertdialog

File Head Base Δ
index-base.css 5.14 KB 5.11 KB ⬆ 0.03 KB (0.61%)
index-vars.css 5.14 KB 5.11 KB ⬆ 0.03 KB (0.61%)
index.css 5.14 KB 5.11 KB ⬆ 0.03 KB (0.61%)

assetlist

File Head Base Δ
index-base.css 7.91 KB 7.90 KB ⬆ 0.01 KB (0.07%)
index-vars.css 7.91 KB 7.90 KB ⬆ 0.01 KB (0.07%)
index.css 7.91 KB 7.90 KB ⬆ 0.01 KB (0.07%)

avatar

File Head Base Δ
index-base.css 5.04 KB 4.99 KB ⬆ 0.06 KB (1.08%)
index-vars.css 5.04 KB 4.99 KB ⬆ 0.06 KB (1.08%)
index.css 5.04 KB 4.99 KB ⬆ 0.06 KB (1.08%)

button

File Head Base Δ
index-base.css 53.04 KB 54.01 KB ⬇ 0.99 KB (-1.78%)
index-theme.css 30.56 KB 30.56 KB -
index-vars.css 83.00 KB 83.96 KB ⬇ 0.99 KB (-1.15%)
index.css 83.00 KB 83.96 KB ⬇ 0.99 KB (-1.15%)
themes/express.css 29.37 KB 29.37 KB -
themes/spectrum.css 29.43 KB 29.43 KB -

calendar

File Head Base Δ
index-base.css 19.29 KB 19.27 KB ⬆ 0.02 KB (0.09%)
index-vars.css 19.29 KB 19.27 KB ⬆ 0.02 KB (0.09%)
index.css 19.29 KB 19.27 KB ⬆ 0.02 KB (0.09%)

card

File Head Base Δ
index-base.css 20.64 KB 20.66 KB ⬇ 0.02 KB (-0.11%)
index-vars.css 20.64 KB 20.66 KB ⬇ 0.02 KB (-0.11%)
index.css 20.64 KB 20.66 KB ⬇ 0.02 KB (-0.11%)

closebutton

File Head Base Δ
index-base.css 15.05 KB 15.19 KB ⬇ 0.14 KB (-0.93%)
index-theme.css 1.26 KB 1.26 KB -
index-vars.css 15.69 KB 15.83 KB ⬇ 0.14 KB (-0.89%)
index.css 15.69 KB 15.83 KB ⬇ 0.14 KB (-0.89%)
themes/express.css 0.96 KB 0.96 KB -
themes/spectrum.css 0.95 KB 0.95 KB -

dialog

File Head Base Δ
index-base.css 14.67 KB 14.64 KB ⬆ 0.03 KB (0.21%)
index-vars.css 14.67 KB 14.64 KB ⬆ 0.03 KB (0.21%)
index.css 14.67 KB 14.64 KB ⬆ 0.03 KB (0.21%)

fieldlabel

File Head Base Δ
index-base.css 6.63 KB 6.54 KB ⬆ 0.09 KB (1.39%)
index-vars.css 6.63 KB 6.54 KB ⬆ 0.09 KB (1.39%)
index.css 6.63 KB 6.54 KB ⬆ 0.09 KB (1.39%)

modal

File Head Base Δ
index-base.css 5.47 KB 5.46 KB ⬆ 0.01 KB (0.13%)
index-vars.css 5.47 KB 5.46 KB ⬆ 0.01 KB (0.13%)
index.css 5.47 KB 5.46 KB ⬆ 0.01 KB (0.13%)

picker

File Head Base Δ
index-base.css 28.37 KB 28.27 KB ⬆ 0.11 KB (0.37%)
index-theme.css 2.50 KB 2.50 KB -
index-vars.css 30.25 KB 30.15 KB ⬆ 0.11 KB (0.34%)
index.css 30.25 KB 30.15 KB ⬆ 0.11 KB (0.34%)
themes/express.css 1.51 KB 1.51 KB -
themes/spectrum.css 1.60 KB 1.60 KB -

popover

File Head Base Δ
index-base.css 15.86 KB 15.72 KB ⬆ 0.14 KB (0.89%)
index-theme.css 0.78 KB 0.78 KB -
index-vars.css 16.01 KB 15.87 KB ⬆ 0.14 KB (0.88%)
index.css 16.01 KB 15.87 KB ⬆ 0.14 KB (0.88%)
themes/express.css 0.69 KB 0.69 KB -
themes/spectrum.css 0.72 KB 0.72 KB -

site

File Head Base Δ
index-base.css 14.26 KB 14.26 KB ⬆ < 0.01 KB (0.01%)
index-vars.css 14.26 KB 14.26 KB ⬆ < 0.01 KB (0.01%)
index.css 14.26 KB 14.26 KB ⬆ < 0.01 KB (0.01%)

stepper

File Head Base Δ
index-base.css 17.14 KB 17.25 KB ⬇ 0.11 KB (-0.64%)
index-theme.css 4.01 KB 4.01 KB -
index-vars.css 20.54 KB 20.65 KB ⬇ 0.11 KB (-0.53%)
index.css 20.54 KB 20.65 KB ⬇ 0.11 KB (-0.53%)
themes/express.css 2.33 KB 2.33 KB -
themes/spectrum.css 2.29 KB 2.29 KB -

toast

File Head Base Δ
index-base.css 8.36 KB 8.39 KB ⬇ 0.03 KB (-0.30%)
index-theme.css 0.87 KB 0.87 KB -
index-vars.css 8.60 KB 8.63 KB ⬇ 0.03 KB (-0.29%)
index.css 8.60 KB 8.63 KB ⬇ 0.03 KB (-0.29%)
themes/express.css 0.75 KB 0.75 KB -
themes/spectrum.css 0.75 KB 0.75 KB -

tooltip

File Head Base Δ
index-base.css 33.71 KB 33.70 KB ⬆ < 0.01 KB (0.01%)
index-theme.css 0.89 KB 0.89 KB -
index-vars.css 33.97 KB 33.96 KB ⬆ < 0.01 KB (0.01%)
index.css 33.97 KB 33.96 KB ⬆ < 0.01 KB (0.01%)
themes/express.css 0.76 KB 0.76 KB -
themes/spectrum.css 0.76 KB 0.76 KB -

treeview

File Head Base Δ
index-base.css 17.06 KB 17.16 KB ⬇ 0.10 KB (-0.57%)
index-vars.css 17.06 KB 17.16 KB ⬇ 0.10 KB (-0.57%)
index.css 17.06 KB 17.16 KB ⬇ 0.10 KB (-0.57%)

underlay

File Head Base Δ
index-base.css 3.27 KB 3.27 KB ⬆ 0.01 KB (0.15%)
index-vars.css 3.27 KB 3.27 KB ⬆ 0.01 KB (0.15%)
index.css 3.27 KB 3.27 KB ⬆ 0.01 KB (0.15%)
* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

@Rajdeepc
Copy link
Collaborator

Rajdeepc commented Aug 30, 2024

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?
cc @pfulton @rubencarvalho

@aramos-adobe aramos-adobe marked this pull request as ready for review August 30, 2024 15:07
@@ -136,13 +138,10 @@ export const Template = ({
isDisabled,
isReadOnly,
customClasses,
customStyles: {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did these go? 😛

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, thanks for the context! I thought it was a forgotten edit 😄

/* Fixed menu height */
.spectrum-Picker--menu-height-fixed {
& + .spectrum-Popover.is-open {
max-block-size: var(--mod-picker-menu-height-fixed, 200px);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for context, did we confirm this default value with design?

@@ -0,0 +1,5 @@
---
"@spectrum-css/picker": major

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the .is-open here? Can't the popover max-block-size be defined independently of being open?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For specificity, I think it's necessary. It also gives other devs more context.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
    }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@aramos-adobe
Copy link
Collaborator Author

@rubencarvalho Ready for review my man!

@pfulton pfulton added the do not merge A flag for a branch indicating it should not be merged. label Sep 17, 2024
@Rajdeepc
Copy link
Collaborator

Rajdeepc commented Oct 7, 2024

@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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this feels like a good use-case to add a mod rather than new API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge A flag for a branch indicating it should not be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants