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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
4bd5e1e
fix(button): text overflows to the next line and button height increases
brandyscarney May 24, 2023
4754703
test(button): add e2e test for button text wrapping
brandyscarney May 24, 2023
c580970
chore(): add updated snapshots
Ionitron May 24, 2023
fc5366f
fix(buttons): reduce padding for slotted toolbar buttons
brandyscarney May 24, 2023
6b0c29e
chore(): add updated snapshots
Ionitron May 24, 2023
acc0715
fix(button): adjust padding and height for small and large buttons
brandyscarney May 24, 2023
a023423
chore(): add updated snapshots
Ionitron May 24, 2023
6516ee4
test(breadcrumbs): remove ion-button from test, use native button
brandyscarney May 24, 2023
a126529
Revert "chore(): add updated snapshots"
brandyscarney May 24, 2023
8092213
Revert "chore(): add updated snapshots"
brandyscarney May 24, 2023
eb77dab
Revert "chore(): add updated snapshots"
brandyscarney May 24, 2023
06909bf
chore(): add updated snapshots
Ionitron May 24, 2023
b5df308
fix(buttons): buttons in toolbar were not aligned properly
brandyscarney May 24, 2023
ffa307f
Revert "test(breadcrumbs): remove ion-button from test, use native bu…
brandyscarney May 24, 2023
1226aa6
test(breadcrumbs): remove wrapping for now
brandyscarney May 24, 2023
13a5655
chore(): add updated snapshots
Ionitron May 24, 2023
88b448e
Revert "chore(): add updated snapshots"
brandyscarney May 24, 2023
84db5ce
Revert "chore(): add updated snapshots"
brandyscarney May 24, 2023
8a8dcff
style(button): rename sass variables for the property they apply to
brandyscarney May 24, 2023
0a41736
Merge branch 'main' into FW-1808-wrap
brandyscarney May 24, 2023
f345d93
fix(button): remove display overrides in inner button
brandyscarney May 25, 2023
94d18c4
fix(button): lower padding top/end for icon buttons
brandyscarney May 25, 2023
1957d51
Merge branch 'main' into FW-1808-wrap
brandyscarney May 25, 2023
46d60af
revert href changes made during testing
brandyscarney May 25, 2023
657109b
test(modal): don't wrap text in buttons
brandyscarney May 25, 2023
360628b
style: lint
brandyscarney May 25, 2023
859f2cb
fix(item): update button height and padding inside of item
brandyscarney May 25, 2023
d67a688
fix(item): decrease padding for outline buttons
brandyscarney May 25, 2023
2fe8870
fix(list-header): set button min-height and reduce padding
brandyscarney May 25, 2023
23f1fe7
fix(button): reduce padding for md for small outline buttons
brandyscarney May 25, 2023
85a2ab2
test(buttono): add e2e test for button in toolbar
brandyscarney May 25, 2023
5c0e69f
fix(button): reduce padding on md
brandyscarney May 26, 2023
6053e0f
fix(button): top and bottom should be the same
brandyscarney May 26, 2023
c4f7bc5
fix: smaller padding
brandyscarney May 26, 2023
162ff26
fix button
brandyscarney May 26, 2023
74bed96
fix(item): button min-height needs to be specified to a fractional am…
brandyscarney Jun 20, 2023
d4a1c99
Revert "test(breadcrumbs): remove wrapping for now"
brandyscarney Jun 20, 2023
c4da8b4
Revert "test(modal): don't wrap text in buttons"
brandyscarney Jun 20, 2023
a63c246
revert(button): change back to white-space nowrap
brandyscarney Jun 20, 2023
b8320d8
test(button): wrap the button text with a class for now
brandyscarney Jun 20, 2023
31a31c1
fix(button): icon size is correct on MD
liamdebeasi Jun 27, 2023
37811ce
Revert "fix(button): icon size is correct on MD"
liamdebeasi Jun 27, 2023
da0ef0d
Revert "fix(item): button min-height needs to be specified to a fract…
liamdebeasi Jun 27, 2023
6f125a7
fix(button): icons are aligned in small buttons
liamdebeasi Jun 27, 2023
7d30a65
lint
liamdebeasi Jun 27, 2023
69c1f84
Merge branch 'feature-7.2' into FW-1808-wrap
brandyscarney Jul 7, 2023
c01a1e1
chore(): add updated snapshots
Ionitron Jul 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions core/src/components/button/button.ios.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

@include margin($button-ios-margin-top, $button-ios-margin-end, $button-ios-margin-bottom, $button-ios-margin-start);

height: #{$button-ios-height};
min-height: #{$button-ios-min-height};

font-size: #{$button-ios-font-size};
font-weight: #{$button-ios-font-weight};
Expand Down Expand Up @@ -92,7 +92,7 @@
--padding-end: #{$button-ios-large-padding-end};
--padding-bottom: #{$button-ios-large-padding-bottom};

height: #{$button-ios-large-height};
min-height: #{$button-ios-large-min-height};

font-size: #{$button-ios-large-font-size};
}
Expand All @@ -104,11 +104,16 @@
--padding-end: #{$button-ios-small-padding-end};
--padding-bottom: #{$button-ios-small-padding-bottom};

height: #{$button-ios-small-height};
min-height: #{$button-ios-small-min-height};

font-size: #{$button-ios-small-font-size};
}

:host(.button-has-icon-only) {
--padding-top: 0;
--padding-bottom: 0;
}


// iOS Round Button
// --------------------------------------------------
Expand Down
18 changes: 9 additions & 9 deletions core/src/components/button/button.ios.vars.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ $button-ios-margin-bottom: 4px !default;
$button-ios-margin-start: 2px !default;

/// @prop - Padding top of the button
$button-ios-padding-top: 0 !default;
$button-ios-padding-top: 13px !default;
brandyscarney marked this conversation as resolved.
Show resolved Hide resolved

/// @prop - Padding end of the button
$button-ios-padding-end: 1em !default;
Expand All @@ -27,8 +27,8 @@ $button-ios-padding-bottom: $button-ios-padding-top !d
/// @prop - Padding start of the button
$button-ios-padding-start: $button-ios-padding-end !default;

/// @prop - Height of the button
$button-ios-height: 3.1em !default;
/// @prop - Minimum height of the button
$button-ios-min-height: 3.1em !default;

/// @prop - Border radius of the button
$button-ios-border-radius: 14px !default;
Expand Down Expand Up @@ -65,7 +65,7 @@ $button-ios-opacity-disabled: .5 !default;
// --------------------------------------------------

/// @prop - Padding top of the large button
$button-ios-large-padding-top: 0 !default;
$button-ios-large-padding-top: 17px !default;

/// @prop - Padding end of the large button
$button-ios-large-padding-end: 1em !default;
Expand All @@ -76,8 +76,8 @@ $button-ios-large-padding-bottom: $button-ios-large-padding-
/// @prop - Padding start of the large button
$button-ios-large-padding-start: $button-ios-large-padding-end !default;

/// @prop - Height of the large button
$button-ios-large-height: 3.1em !default;
/// @prop - Minimum height of the large button
$button-ios-large-min-height: 3.1em !default;

/// @prop - Border radius of the large button
$button-ios-large-border-radius: 16px !default;
Expand All @@ -90,7 +90,7 @@ $button-ios-large-font-size: 20px !default;
// --------------------------------------------------

/// @prop - Padding top of the small button
$button-ios-small-padding-top: 0 !default;
$button-ios-small-padding-top: 4px !default;

/// @prop - Padding end of the small button
$button-ios-small-padding-end: .9em !default;
Expand All @@ -101,8 +101,8 @@ $button-ios-small-padding-bottom: $button-ios-small-padding-
/// @prop - Padding start of the small button
$button-ios-small-padding-start: $button-ios-small-padding-end !default;

/// @prop - Height of the small button
$button-ios-small-height: 2.1em !default;
/// @prop - Minimum height of the small button
$button-ios-small-min-height: 2.1em !default;

/// @prop - Border radius of the small button
$button-ios-small-border-radius: 6px !default;
Expand Down
11 changes: 8 additions & 3 deletions core/src/components/button/button.md.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

@include margin($button-md-margin-top, $button-md-margin-end, $button-md-margin-bottom, $button-md-margin-start);

height: #{$button-md-height};
min-height: #{$button-md-min-height};

font-size: #{$button-md-font-size};
font-weight: #{$button-md-font-weight};
Expand Down Expand Up @@ -97,7 +97,7 @@
--padding-end: #{$button-md-large-padding-end};
--padding-bottom: #{$button-md-large-padding-bottom};

height: #{$button-md-large-height};
min-height: #{$button-md-large-min-height};

font-size: #{$button-md-large-font-size};
}
Expand All @@ -108,11 +108,16 @@
--padding-end: #{$button-md-small-padding-end};
--padding-bottom: #{$button-md-small-padding-bottom};

height: #{$button-md-small-height};
min-height: #{$button-md-small-min-height};

font-size: #{$button-md-small-font-size};
}

:host(.button-has-icon-only) {
--padding-top: 0;
--padding-bottom: 0;
}


// MD strong Button
// --------------------------------------------------
Expand Down
20 changes: 10 additions & 10 deletions core/src/components/button/button.md.vars.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,19 @@ $button-md-margin-bottom: 4px !default;
$button-md-margin-start: 2px !default;

/// @prop - Padding top of the button
$button-md-padding-top: 0 !default;
$button-md-padding-top: 8px !default;

/// @prop - Padding end of the button
$button-md-padding-end: 1.1em !default;

/// @prop - Padding bottom of the button
$button-md-padding-bottom: 0 !default;
$button-md-padding-bottom: $button-md-padding-top !default;

/// @prop - Padding start of the button
$button-md-padding-start: 1.1em !default;

/// @prop - Height of the button
$button-md-height: 36px !default;
/// @prop - Minimum height of the button
$button-md-min-height: 36px !default;

/// @prop - Border radius of the button
$button-md-border-radius: 4px !default;
Expand Down Expand Up @@ -63,7 +63,7 @@ $button-md-opacity-disabled: .5 !default;
// --------------------------------------------------

/// @prop - Padding top of the large button
$button-md-large-padding-top: 0 !default;
$button-md-large-padding-top: 14px !default;

/// @prop - Padding end of the large button
$button-md-large-padding-end: 1em !default;
Expand All @@ -74,8 +74,8 @@ $button-md-large-padding-bottom: $button-md-large-padding
/// @prop - Padding start of the large button
$button-md-large-padding-start: $button-md-large-padding-end !default;

/// @prop - Height of the large button
$button-md-large-height: 2.8em !default;
/// @prop - Minimum height of the large button
$button-md-large-min-height: 2.8em !default;

/// @prop - Font size of the large button
$button-md-large-font-size: 20px !default;
Expand All @@ -85,7 +85,7 @@ $button-md-large-font-size: 20px !default;
// --------------------------------------------------

/// @prop - Padding top of the small button
$button-md-small-padding-top: 0 !default;
$button-md-small-padding-top: 4px !default;

/// @prop - Padding end of the small button
$button-md-small-padding-end: .9em !default;
Expand All @@ -96,8 +96,8 @@ $button-md-small-padding-bottom: $button-md-small-padding
/// @prop - Padding start of the small button
$button-md-small-padding-start: $button-md-small-padding-end !default;

/// @prop - Height of the small button
$button-md-small-height: 2.1em !default;
/// @prop - Minimum height of the small button
$button-md-small-min-height: 2.1em !default;

/// @prop - Font size of the small button
$button-md-small-font-size: 13px !default;
Expand Down
24 changes: 17 additions & 7 deletions core/src/components/button/button.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@

text-align: center;
text-decoration: none;
text-overflow: ellipsis;

// TODO(FW-4599): change to normal
white-space: nowrap;

user-select: none;
Expand Down Expand Up @@ -114,8 +114,6 @@
:host(.button-block) .button-native {
@include margin-horizontal(0);

display: block;

width: 100%;

clear: both;
Expand All @@ -138,8 +136,6 @@
:host(.button-full) .button-native {
@include margin-horizontal(0);

display: block;

width: 100%;

contain: content;
Expand All @@ -159,12 +155,17 @@
@include padding(var(--padding-top), var(--padding-end), var(--padding-bottom), var(--padding-start));
@include text-inherit();

display: block;
display: flex;

position: relative;

align-items: center;

width: 100%;
height: 100%;

min-height: inherit;

transition: var(--transition);

border-width: var(--border-width);
Expand Down Expand Up @@ -210,11 +211,20 @@
}


// Button Slots
// --------------------------------------------------

::slotted([slot=start]),
::slotted([slot=end]) {
flex-shrink: 0;
}
averyjohnston marked this conversation as resolved.
Show resolved Hide resolved


// Button Icons
// --------------------------------------------------

::slotted(ion-icon) {
font-size: 1.4em;
font-size: 1.35em;
Comment on lines -217 to +227
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 •
edited
Loading

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.

pointer-events: none;
}

Expand Down
Binary file modified ...n/test/icon/button.e2e.ts-snapshots/button-icon-ios-ltr-Mobile-Chrome-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified .../test/icon/button.e2e.ts-snapshots/button-icon-ios-ltr-Mobile-Firefox-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...n/test/icon/button.e2e.ts-snapshots/button-icon-ios-ltr-Mobile-Safari-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...n/test/icon/button.e2e.ts-snapshots/button-icon-ios-rtl-Mobile-Chrome-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified .../test/icon/button.e2e.ts-snapshots/button-icon-ios-rtl-Mobile-Firefox-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...n/test/icon/button.e2e.ts-snapshots/button-icon-ios-rtl-Mobile-Safari-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...on/test/icon/button.e2e.ts-snapshots/button-icon-md-ltr-Mobile-Chrome-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...n/test/icon/button.e2e.ts-snapshots/button-icon-md-ltr-Mobile-Firefox-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...on/test/icon/button.e2e.ts-snapshots/button-icon-md-ltr-Mobile-Safari-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...on/test/icon/button.e2e.ts-snapshots/button-icon-md-rtl-Mobile-Chrome-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...n/test/icon/button.e2e.ts-snapshots/button-icon-md-rtl-Mobile-Firefox-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...on/test/icon/button.e2e.ts-snapshots/button-icon-md-rtl-Mobile-Safari-linux.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading