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(button): use clamp for font sizes on circle shape #29200

Merged
merged 16 commits into from Apr 3, 2024

Conversation

thetaPC
Copy link
Contributor

@thetaPC thetaPC commented Mar 21, 2024

Issue number: internal


What is the current behavior?

The font sizes for the icons are not clamping. This can lead to the icons getting too big if the font-size to the component is a large value.

What is the new behavior?

  • The font sizes for icons are now clamped for ios and md.
  • Updated the small circle button's font size after reviewing iOS native.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Here are some comparisons between native and the implementation. The red icon is the implementation and the white icon is the native app:

Default:
https://github.com/ionic-team/ionic-framework/assets/13530427/b4aaedea-40ec-4378-ae26-8d7962b7ff6f

Max font size:
https://github.com/ionic-team/ionic-framework/assets/13530427/286f76f3-fd31-44b7-bba1-38eba9a6aeb3

Min font sizes:
https://github.com/ionic-team/ionic-framework/assets/13530427/8dc4b4bd-035d-44b7-863e-ba7b7947bde1

@github-actions github-actions bot added the package: core @ionic/core package label Mar 21, 2024
@thetaPC thetaPC marked this pull request as ready for review March 21, 2024 19:59
@thetaPC thetaPC requested review from brandyscarney and a team as code owners March 21, 2024 19:59
@sean-perkins sean-perkins requested review from liamdebeasi and removed request for brandyscarney March 25, 2024 15:05
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.

I know we discussed this internally today, but I'm adding my comments here so we can keep track of changes.

core/src/components/button/button.ios.scss Outdated Show resolved Hide resolved
core/src/components/button/button.ios.scss Outdated Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The small icon did not have the correct size. The screenshot change is due to updating the value to be correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The small icon did not have the correct size. The screenshot change is due to updating the value to be correct.

* The values were provided through a native iOS app.
* min font size: 13px, default font size: 17px, max font size: 38px
*/
font-size: dynamic-font-clamp(0.63, 20.93px, 1.84, 1em);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the default font size 20.93 when it says the default font size is 17 in the comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is due to ion-button, the parent, having a different font size than the typical 16px.

* The values were provided through a native iOS app.
* min font size: 13.5px, default font size: 20px, max font size: 40.5px
*/
font-size: dynamic-font-clamp(0.85, 16px, 2.53, 1em);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the default size 16 when it says 20 in the comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is due to ion-button, the parent, having a different font size than the typical 16px.

@thetaPC thetaPC requested a review from liamdebeasi April 2, 2024 17:51
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.

Thank you!

@liamdebeasi liamdebeasi changed the title refactor(button): use clamp for font sizes on circle shape fix(button): use clamp for font sizes on circle shape Apr 3, 2024
@liamdebeasi
Copy link
Contributor

Also, this is fix not refactor because there are visual changes.

@thetaPC thetaPC merged commit 4d6edee into feature-8.0 Apr 3, 2024
42 checks passed
@thetaPC thetaPC deleted the FW-6017-font-sizes branch April 3, 2024 21:28
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.

None yet

3 participants