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(alert): allow 5.5 items to show in alert-mode select popup in md #23976

Merged
merged 2 commits into from
Sep 24, 2021

Conversation

averyjohnston
Copy link
Contributor

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: Resolves #23760

In md mode, alert-style ion-selects with more than 5 items don't look scrollable, due to the alert content having the height for exactly 5 items.

What is the new behavior?

  • Alert content height increased to allow for 5.5 items (going off the MD spec item height of 48px). The cut-off item makes it clear that the list can be scrolled to reveal more items.

Does this introduce a breaking change?

  • Yes
  • No

Other information

@github-actions github-actions bot added the package: core @ionic/core package label Sep 23, 2021
Co-authored-by: Liam DeBeasi <[email protected]>
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Great job! One final thing: We try to word our commits such that they describe what issue was resolved rather than what changed in the code. So one way you could word this is:

fix(alert): made it easier to determine if an alert is scrollable in MD mode

However you want to word it is up to you, as long as we convey what issue got fixed.

@averyjohnston
Copy link
Contributor Author

Great job! One final thing: We try to word our commits such that they describe what issue was resolved rather than what changed in the code.

🤔 The contributing doc says to do the reverse: https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#commit-message-guidelines

describe what the commit does, not what issue it relates to or fixes

@liamdebeasi
Copy link
Contributor

Sorry that's what I meant -- My comment was worded in a confusing way. This PR makes it easier to tell if an alert is scrollable, so describing the commit in that manner is something we should shoot for. (At the end of the day it's not an exact science, so if you have a commit message that you think is descriptive for Ionic developers then that's fine).

Thanks for pointing that out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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