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

feat(button): allow button to increase in height when text wraps #27547

Merged
merged 47 commits into from
Jul 7, 2023

Conversation

brandyscarney
Copy link
Member

@brandyscarney brandyscarney commented May 24, 2023

Issue number: N/A - this does not completely resolve an issue but it enables users to opt-in to having text wrap in a button by setting a minimum height


What is the current behavior?

The current behavior when text is really long in a button is:

  • Default buttons expand in width until part of the text (and button) is off the screen and not in the visible viewport
  • Block and full buttons horizontally align the text in the center and overflow it on both sides (but the overflow is not visible so the text is cut off at the beginning and end)

What is the new behavior?

Allow the button height to increase when text wraps and add some padding so that buttons with wrapped text still look nice. This does NOT wrap the text in a button by default. That will be done in FW-4599.

  • Removed text-overflow: ellipsis since this does not have any effect
  • Changes height setting to min-height on all button types (small, large, default) and buttons inside of an item, toolbar or list header
  • Increases padding-top and padding-bottom on the buttons so that overflowing buttons have padding around them
  • Changes .button-native display property from block to flex in order for anchor tags (<ion-button href="#"> to align their text vertically
  • Sets flex-shrink: 0 on slotted start/end elements to prevent icons (and other elements) from shrinking to make room for the text
  • Adds e2e test for button wrapping including the different types of buttons that were changed by this PR
    • Adds ion-text-wrap to the ion-button elements used in this test to verify the height / padding changes are working as desired (to be removed with FW-4599)
  • Screenshot diffs are in the following PR: chore(): add updated snapshots #27561

Does this introduce a breaking change?

  • Yes
  • No

Other information

This does NOT wrap the text in a button by default. It only enables buttons to look nicer and auto adjust their height/padding when the text is wrapping.

After internal discussion we decided that automatically making the text wrap inside of a button may have undesired effects on existing apps. For example, if someone has a button inside of a list header with a long label, the button will now wrap if it has a space or dash in the text content.

Developers should set ion-text-wrap on the ion-button to opt-in to text wrapping in a button, and this will become the default as part of FW-4599 (the next major release).

@stackblitz
Copy link

stackblitz bot commented May 24, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@github-actions github-actions bot added the package: core @ionic/core package label May 24, 2023
@brandyscarney brandyscarney marked this pull request as ready for review June 20, 2023 21:44
@brandyscarney brandyscarney requested review from a team and liamdebeasi and removed request for a team June 22, 2023 14:21
@liamdebeasi liamdebeasi force-pushed the FW-1808-wrap branch 3 times, most recently from 8548112 to 31a31c1 Compare June 27, 2023 21:19
Comment on lines -217 to +227
font-size: 1.4em;
font-size: 1.35em;
Copy link
Contributor

Choose a reason for hiding this comment

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

font-size: 1.4em with min-height: 25px was triggering a browser rendering issue where the text in a button was misaligned with the icon. We did not notice this before because the button height was height: 25px. Now that the height of the button is only the minimum height we are now seeing this bug. The issue appears to be that the height of the icon is causing the text to be misaligned.

We tried to fix it by setting min-height: 25.19px on ion-button. This fixed the issue for Chrome and Firefox, but it made the problem worse for Safari. Instead, we found that by using a slightly smaller icon size we can get the icon and the text to be aligned.

Copy link
Contributor

Choose a reason for hiding this comment

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

It also turns out that the icons were not aligned with the text in main even with height: 25px. Looks like min-height: 25px made the issue worse enough that we are noticing it now.

main branch
image image

In main, there are 2px of blank space at the bottom of the text relative to the bottom of the icon. In branch, there is 1px of blank space above and below the text.

Copy link
Contributor

@liamdebeasi liamdebeasi Jun 27, 2023

Choose a reason for hiding this comment

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

Note that some buttons may have an "off by 1" alignment due to fonts that are odd (i.e. 13px). This is a problem in main too, so this change does not worsen the issue.

Example: The small button on MD has a font size of 12px. 1.4em * 12 = 16.8 (which gets rounded to 17px). As a result small buttons in main have an off by 1 alignment issue.

However, the large button has a font size of 20px. 1.35em (the new icon size) * 20 = 27. As a result, large buttons will have an off by 1 alignment issue instead of small buttons.

Long term we should avoid odd font sizes.

Copy link
Contributor

Choose a reason for hiding this comment

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

The small items in an item on MD also have a font size of 12px which results in 1.4 * 12 = 16.8 (rounded to 17px). I considered changing this to be 13px to avoid changing the icon size. However, this caused larger visual regressions than I was comfortable with, so I opted to stick with the above approach.

@brandyscarney brandyscarney merged commit 6fe716f into feature-7.2 Jul 7, 2023
@brandyscarney brandyscarney deleted the FW-1808-wrap branch July 7, 2023 19:04
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: long text with max width not truncating in button
4 participants