Skip to content

feat(checkbox): default size and appearance#29236

Merged
sean-perkins merged 5 commits intosp/ROU-4822from
sp/ROU-4822-default
Mar 29, 2024
Merged

feat(checkbox): default size and appearance#29236
sean-perkins merged 5 commits intosp/ROU-4822from
sp/ROU-4822-default

Conversation

@sean-perkins
Copy link
Contributor

@sean-perkins sean-perkins commented Mar 28, 2024

Issue number: N/A


What is the current behavior?

What is the new behavior?

  • Adds the default display of a checkbox with the "ionic" theme.
    • The default size of a checkbox with the theme is 24px.
  • Adds global sass files specific to the ionic theme.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Split from #29213. I will note differences in comments on this PR.

@github-actions github-actions bot added the package: core @ionic/core package label Mar 28, 2024
Copy link
Contributor Author

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

Feedback/notes for reviewers.

The key pieces to review here are:

  1. Did I make any silly mistakes in the code.
  2. Does the checkbox render at 24px by default

Follow-up PRs will address the other sizes, shapes, states (focus/active/disabled), etc.

$checkbox-ionic-size: 24px !default;

/// @prop - The background color of the checkbox icon when the checkbox is unchecked
$checkbox-ionic-icon-background-color-off: $background-color-step-450 !default;
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 border color is based from the Figma design color of #9A9A9A.

Note: OS can customize the color palette to actually apply their desired color.

$background-color-step-450 evaluates to #8C8C8C and provides a good visual default to an existing Ionic Framework variable.

Copy link
Member

@brandyscarney brandyscarney Mar 28, 2024

Choose a reason for hiding this comment

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

any reason for this over --ion-color-step-400: #999999;?

diff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm ok with updating to step-400 👍 I had looked at both and simply committed. Without the design tokens connected from Figma, we will have to pick what is reasonably close & adjust as that architecture exists.


/// @prop - The border radius of the checkbox icon.
/// With a default size of 24px, the border radius is calculated as 24px / 6 = 4px
$checkbox-ionic-border-radius: calc(var(--size) / 6) !default;
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 original border radius value in the linked PR was incorrect. It was copied from Material Design, but in the Figma file we can see the desired border radius is:

  • When the checkbox is 24px the border radius is 4px
  • When the checkbox is small (16px) the border radius is 2px.

With the current calculation, we are close enough:

  • When the checkbox is 24px the border radius is 4px
  • When the checkbox is small (16px) the border radius is 2.667px

OS can target the --border-radius if they need to hard-set the border radius to 2px for the small size.

This approach of basing the border radius on the size, provides a better visual appearance for when a developer customizes the size beyond the default sizes.

Copy link
Member

Choose a reason for hiding this comment

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

If we wanted to get it to match on both small and large we could do this:

Suggested change
$checkbox-ionic-border-radius: calc(var(--size) / 6) !default;
$checkbox-ionic-border-radius: calc(var(--size) / 4 - 2px) !default;

but that's optional here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it.

@sean-perkins sean-perkins changed the title Sp/rou 4822 default feat(checkbox): default size and appearance Mar 28, 2024
Discussed with Liam & Brandy, we are going to simplify these names since they aren't specific to the icon.

:host {
// Border
--border-radius: #{$checkbox-ionic-border-radius};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed internally with Liam & Brandy. You will notice the naming convention here is different than iOS/MD. We've decided to remove the -icon from the name. It didn't make sense.

We left iOS/MD alone, this only applies to the ionic theme.

@sean-perkins sean-perkins marked this pull request as ready for review March 28, 2024 20:33
Copy link
Member

@brandyscarney brandyscarney left a comment

Choose a reason for hiding this comment

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

Looks good, added an optional change to make the border radius match the design more closely.

@sean-perkins sean-perkins merged commit 6a9dd99 into sp/ROU-4822 Mar 29, 2024
@sean-perkins sean-perkins deleted the sp/ROU-4822-default branch March 29, 2024 01:54
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.

2 participants