fix(button): update icon size to the default size

[joselrio]

Issue number: ROU-4815

---

What is the current behavior?

• Ionic theme, Icon size was 14px to the default button size;

What is the new behavior?

• Ionic theme, Icon size has now 16px to the default button size;

NOTE:

• Tests related... this a fix on top of PR of ROU-4815 branch and as was mentioned there, tests will be implemented under a task that will be then created specifically for it.

[thetaPC]

Also the PR title needs to be updated to follow the conventional commits structure. Example: fix(button): update icon size to the default size

Notice that the example:

1. doesn't wrap the type with brackets
2. doesn't have capital words
3. uses present tense
4. doesn't use the word fix in the description since it's already mentioned through the type

You can also find more details under the Ionic Framework PR Process guide.

[thetaPC]

I noticed that there are two instances of this. What's the difference between this one and the grouping further in the file?

[joselrio]

You're right @thetaPC 🤦. Thank you!
Already fixed!

[thetaPC]

@joselrio one last thing on the PR title that needs to change, remove the period. The PR titles shouldn't have punctuation marks. Thank you!

[thetaPC]

We also need a test within core/src/components/button/test/icon/button.e2e.ts to verify that there will be no visual regressions moving forward. I recommend updating the current one to include these modes, modes: ['ios', 'md', 'ionic-md'], within the configs function.

configs({ modes: ['ios', 'md', 'ionic-md'] }).forEach(({ title, screenshot, config }) => {
test.describe(title('button: icon'), () => {
    test('should not have visual regressions', async ({ page }) => {
      await page.goto(`/src/components/button/test/icon`, config);

      await page.setIonViewport();

      await expect(page).toHaveScreenshot(screenshot(`button-icon`));
    });
  });
});

[joselrio]

@thetaPC as was mentioned at the base PR for the button, tests will be done under a new task that will be created specific for it.

[thetaPC]

@joselrio Understood on the test. Please update the PR description to mention that tests will be done through another PR.

[joselrio]

@thetaPC I think you did not understood the difference between em and rem units. Icons has already sizes you mentioned due to font-size applied at the context and the usage of em units for the icons font-size.

Note that (example), If button font-size: 14px, and icon:

• font-size: 1em, then icon will have 14px as well;
• font-size: 0.5em, then icon will have 7px;
• font-size: 1.5em, then icon will have 21px;

For the default (medium) size there is the need to be different since font-size is 14px (converted into rem(s)) and the icon should have 16px (on this particular case, also converted into rem(s)).

Take a look at the images below please:

xsmall

small

default/medium

large

xlarge

Let me know if you have any doubt about it.

[thetaPC]

Discussed offline:

The icon sizes don't behave as expected when the font increases through the app. This is what it looks like when a user increases the font size, the left side is default and the left side is large. Notice how the large icon is smaller than default.

This PR is on hold until the design team can determine next steps.

[thetaPC]

Closing this PR due to my previous comment regarding having to wait for the design team. This will be re-visited at a later date.

[thetaPC]

Disregard my previous comment, we want to leave this PR open and then make a new with design's decision.

[thetaPC]

LGTM, this is accomplishing its intended result. The cause for delay was a lack of understanding on how dynamic font manual testing should have happened.

[sean-perkins]

We discussed as a larger team and I included some information in our Slack as well.

These changes look good when tested with dynamic font scaling. The maximum scale for dynamic font scaling is 310%.