ROU-4822 - [feat](checkbox): Add new checkbox features under the scope of the new ionic theme.#29213
ROU-4822 - [feat](checkbox): Add new checkbox features under the scope of the new ionic theme.#29213JoaoFerreira-FrontEnd wants to merge 4 commits intonextfrom
Conversation
- add features for ionic theme of checkbox;
JoaoFerreira-FrontEnd
requested review from
a team,
averyjohnston,
liamdebeasi,
sean-perkins and
thetaPC
as code owners
github-actions
bot
added
package: core
|
|
||
| border: 2px solid $checkbox-ionic-focus-ring-color; | ||
|
|
||
| /* stylelint-disable-next-line property-disallowed-list */ |
There was a problem hiding this comment.
Ionic Framework provides a @include border-radius mixin. This is why we throw the stylelint error.
This usage should be updated to:
@include border-radius(var(--border-radius));
| /** | ||
| * Set to `"soft"` for a checkbox with more rounded corners. | ||
| */ | ||
| @Prop({ reflect: true }) shape?: 'soft' | 'rectangular' = 'soft'; |
There was a problem hiding this comment.
I would recommend removing the { reflect: true } for now. This applies to both usages.
This is some history here, but Ionic Framework currently uses reflection of CSS classes on the element host, to use for the styling selectors. We should eventually move away from this, as reflecting the attributes is actually more compliant in SSR environments (mark-up provided by the developer matches the rendered output).
Since we are not using these attributes in our current style implementation, we are invoking more JS in Stencil's virtual dom to reflect the attributes to the element.
| <svg class="checkbox-icon" viewBox="0 0 24 24" part="container"> | ||
| {path} | ||
| </svg> | ||
| {theme === 'ionic' && <div part="focus-ring" class="focus-ring"></div>} |
There was a problem hiding this comment.
Be sure to document this shadow part at the top of the file. You can checkout what was done for button on the latest next branch. We should make it clear this shadow part is only available in the ionic theme.
| @@ -0,0 +1,43 @@ | |||
| import { expect } from '@playwright/test'; | |||
There was a problem hiding this comment.
I'm working to correct an issue in our test generators: #29206, but after that is merged we should be able to add ionic-md to existing test cases vs. having separate test cases specifically for the ionic theme.
The advantage is we aren't maintaining entire test suites specific to a theme, but can run the same test suite against many different themes.
There was a problem hiding this comment.
Yes, that makes sense, I did just maintain a page with everything just for the stage, so the designers can see everything side by side.
Issue number: N/A --------- <!-- Please do not submit updates to dependencies unless it fixes an issue. --> <!-- Please try to limit your pull request to one type (bugfix, feature, etc). Submit multiple pull requests if needed. --> ## What is the current behavior? <!-- Please describe the current behavior that you are modifying. --> ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> - 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 - [x] No <!-- If this introduces a breaking change: 1. Describe the impact and migration path for existing applications below. 2. Update the BREAKING.md file with the breaking change. 3. Add "BREAKING CHANGE: [...]" to the commit description when merging. See https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#footer for more information. --> ## Other information <!-- Any other information that is important to this PR such as screenshots of how the component looks before and after the change. --> Split from #29213. I will note differences in comments on this PR.
3 participants
Issue number: ROU-4822
What is the new behavior?
Does this introduce a breaking change?
Preview