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

bug/feat: ion-select with more than 5 items lacks visual indicator on Material Theme #23760

Closed
4 of 6 tasks
cmaas opened this issue Aug 11, 2021 · 5 comments · Fixed by #23976
Closed
4 of 6 tasks

bug/feat: ion-select with more than 5 items lacks visual indicator on Material Theme #23760

cmaas opened this issue Aug 11, 2021 · 5 comments · Fixed by #23976
Labels
help wanted a good issue for the community package: core @ionic/core package type: bug a confirmed bug report
Milestone

Comments

@cmaas
Copy link

cmaas commented Aug 11, 2021

Prequisites

Ionic Framework Version

  • v4.x
  • v5.x
  • v6.x

Current Behavior

An ion-select with more than 5 items has no visual indicator that the selection area is scrollable, but only for the Material Theme. The iOS theme shows half of the 6th item, which serves as a visual indicator that the whole area is scrollable.

The reason for this is simple:

.alert-radio-group.sc-ion-alert-ios, .alert-checkbox-group.sc-ion-alert-ios {
  max-height: 240px;
}
.alert-tappable.sc-ion-alert-ios {
    height: 44px;
}

240px / 44px = 5,45 items to show.

.alert-radio-group.sc-ion-alert-md, .alert-checkbox-group.sc-ion-alert-md {
    max-height: 240px;
}

.alert-tappable.sc-ion-alert-md {
    height: 48px;
}

240px / 48px = exactly 5 items.

Solution:

.alert-radio-group.sc-ion-alert-md, .alert-checkbox-group.sc-ion-alert-md {
    max-height: 264px;
}

Because 264px / 48px = 5.5 items.

Sure, I can override this in app.css, but I think it makes sense as a good default for all ionic apps, since it is confusing for users.

Expected Behavior

Show 5.5 items in Material Theme

Steps to Reproduce

<ion-item>
          <ion-label>Music</ion-label>
          <ion-select value="03">
            <ion-select-option value="01">Alice in Chains</ion-select-option>
            <ion-select-option value="02">Green Day</ion-select-option>
            <ion-select-option value="03">Nirvana</ion-select-option>
            <ion-select-option value="04">Pearl Jam</ion-select-option>
            <ion-select-option value="05">Smashing Pumpkins</ion-select-option>
            <ion-select-option value="06">Soundgarden</ion-select-option>
            <ion-select-option value="07">Stone Temple Pilots</ion-select-option>
          </ion-select>
 </ion-item>

Or see here and toggle between themes: https://ionicframework.com/docs/api/select

Code Reproduction URL

https://ionicframework.com/docs/api/select

Ionic Info

Package.json:

"@ionic/core": "5.6.12"

Additional Information

list-ios

list-md

@ionitron-bot ionitron-bot bot added the triage label Aug 11, 2021
@willmartian willmartian added package: core @ionic/core package type: bug a confirmed bug report labels Aug 11, 2021
@ionitron-bot ionitron-bot bot removed the triage label Aug 11, 2021
@willmartian willmartian added the help wanted a good issue for the community label Aug 11, 2021
@ionitron-bot
Copy link

ionitron-bot bot commented Aug 11, 2021

This issue has been labeled as help wanted. This label is added to issues that we believe would be good for contributors.

If you'd like to work on this issue, please comment here letting us know that you would like to submit a pull request for it. This helps us to keep track of the pull request and make sure there isn't duplicated effort.

For a guide on how to create a pull request and test this project locally to see your changes, see our contributing documentation.

Thank you!

@willmartian
Copy link
Contributor

willmartian commented Aug 11, 2021

Hey @cmaas, thanks for this issue! I agree that this is a UX problem that should be fixed.

If you or another community member would like to work on this, I would also suggest referring to the Material Design spec to see if our sizes for the dialog and list items match their guidelines: https://www.material.io/components/dialogs#specs

@cmaas
Copy link
Author

cmaas commented Aug 11, 2021

@willmartindev

Sure, I'm happy to help. I looked up the specs.

Ionic's list item height is correct (48px). The specs expect 48dp (https://www.material.io/components/lists#specs). So it makes no sense to reduce the height of an item.

Ionic has a somewhat arbitrary max-height of 240px for dialogs. See $alert-md-content-max-height in

$alert-md-content-max-height: 240px !default;

The specs for the height of dialogs is defined as:

Dialogs should maintain a minimum 48dp distance from the leading and trailing edges of a screen. On small screens, dialogs scale vertically until reaching 48dp from the screen’s top and bottom edge. On larger displays, dialogs can increase in size to a maximum width of 560dp.

Source: https://material.io/components/dialogs#behavior → Section Scaling and adaptation

Examples throughout the specs usually show a cropped list item if the content is scrollable. See here:

But the specs don't explicitly mention a height that shows only half of an element.


Easy solution: Set $alert-md-content-max-height to 264px, because this is item-height of 48px * 5.5 = 264px. Then it would be like the dialog on iOS.

More complex solution: Discuss if you want to update dialogs/alerts to the above Material spec: A minimum top and bottom padding of 48px and a max-height of up to 560px. This probably requires a lot more testing etc., because it depends whether the alert of an ion-select was opened within another dialog.

I think a good approach would be to implement a more sensitive max-height of 264px first and gradually adjust the height of a dialog in other versions. Imho, the goal is to give users a visual indicator that the list is scrollable. Whether or not the screen could be filled a bit better is a different matter.

@liamdebeasi liamdebeasi added type: bug a confirmed bug report and removed type: bug a confirmed bug report labels Sep 21, 2021
@averyjohnston
Copy link
Contributor

Hi @cmaas, thank you very much for the report and deep dive. I've resolved this via #23976 and the fix will be available in a future release of Ionic.

I decided to go with your suggestion to just increase the list height for now, and I also made a separate issue for the mismatch with the MD spec: #23977

@liamdebeasi liamdebeasi added this to the 5.8.2 milestone Sep 27, 2021
@ionitron-bot
Copy link

ionitron-bot bot commented Oct 27, 2021

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Oct 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted a good issue for the community package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants