feat(button): add ionic theme implementation #29187
Conversation
There was a problem hiding this comment.
Tested locally, really great work here!
I've commented on some code style and testing best practices.
When making changes to some of the code documentation, you will need to run npm run build locally to regenerate certain files.
I need to run, but I can provide information on generating screenshot in a follow-up comment later.
Co-authored-by: Sean Perkins <13732623+sean-perkins@users.noreply.github.com>
|
@sean-perkins amazing feedback! Super useful to get deeper context... |
Co-authored-by: Brandy Carney <brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Carney <brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Carney <brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Carney <brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Carney <brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Carney <brandyscarney@users.noreply.github.com>
Co-authored-by: Maria Hutt <thetaPC@users.noreply.github.com>
Co-authored-by: Maria Hutt <thetaPC@users.noreply.github.com>
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. --> Adding new e2e tests into Ionic Framework can be difficult for new contributors when it comes to knowing where existing test cases exist and how to additionally add a new theme or option to the test. To avoid a lot of back and forth feedback in Github comments, I split up the test on the original PR to help demonstrate how to match existing test structures. ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> - Removes the proposed test specific to the Ionic theme. - Migrates the tests into the existing e2e files for button, adding specific test cases that only apply to the ionic theme. - Added screenshot references In this work, I tried to correct some legacy folder naming: - `button/test/round` → `button/test/shape` - For `fill="clear"` I updated the screenshot names, but left the folder structure. There is more work here to organize `fill="outline"` and it is not required at this time. ## 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. --> I left the HTML template for the ionic theme test alone. I figure this is helpful to the developer, until the primary PR is approved. Then it could be removed if it is no longer useful.
sean-perkins
changed the title
|
@thetaPC missing tests will be done under another task that will be created. This PR should not be blocked for now due to those missing tests. Thank you :) |
This whitespace change should not exist.
There was a problem hiding this comment.
Great work on this 👏
I've updated the use of the border-radius property for our @include border-radius mixin. I've also updated a few comment blocks in the Sass stylesheet, but we can continue to update that as additional features are added.
I believe this PR is ready for merge. I've linked technical debt tasks after another PR for test infrastructure is merged. When that happens, a test template will be removed and new screenshots for test cases will be added.
We should provide the Design team with a build of these changes, so they can validate the accuracy of the implementation as well, unless they have already looked and approved the implementation separately.
|
@joselrio I noticed that Sean's PR was merged into this one. But I'm not seeing his changes here. |
sean-perkins
dismissed
brandyscarney’s stale review
Spoke with Brandy internally and we split up some current priorities. I grabbed the review so she can tackle another priority.
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](#29187) and as was mentioned there, tests will be implemented under a task that will be then created specifically for it.
Issue number: ROU-4815
What is the new behavior?
ion-button.rectangularshape to the button.xsmallandxlarge.focus-ringCSS shadow part for the ionic theme.Does this introduce a breaking change?
Preview