Skip to content

fix(button): activated outline button in toolbar no longer blends into background on MD dark mode - #29216

Merged
averyrousseau merged 25 commits into
mainfrom
FW-5721
Apr 2, 2024
Merged

fix(button): activated outline button in toolbar no longer blends into background on MD dark mode#29216
averyrousseau merged 25 commits into
mainfrom
FW-5721

Conversation

@averyrousseau

@averyrousseau averyrousseau commented Mar 25, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Issue number: N/A

What is the current behavior?

When using an outline-style button in a toolbar on md dark mode, the activated style causes the button to become invisible.

Steps to repro in main:

  1. Update the css-variables themes test to use the latest dark theme styles (at least for v7) by replacing the contents of this file with the styles from the docs.
  2. Host the project locally and navigate to the test at themes/test/css-variables/index.html.
  3. Switch to dark mode within the page. Note that the activated outline button isn't visible:
    image

What is the new behavior?

  • Button styles corrected. The colors were correct for ios mode, but had to be flipped for md, so I pulled the relevant styles into the mode-specific stylesheets.
  • CSS variables test has also been updated to use the same dark mode CSS as in the docs, as per the repro steps above. Let me know if you would rather this be split into a separate PR.

Changes to the toolbar test were split into a separate PR to keep this one clean and ensure the screenshot changes can be reviewed more effectively: #29231

Does this introduce a breaking change?

  • Yes
  • No

Other information

@github-actions github-actions Bot added the package: core @ionic/core package label Mar 25, 2024
@averyrousseau averyrousseau changed the title fix(button): activated button in toolbar no longer blends into background on dark mode fix(button): activated outline button in toolbar no longer blends into background on dark mode Mar 25, 2024
@averyrousseau
averyrousseau marked this pull request as ready for review March 25, 2024 19:36
@averyrousseau
averyrousseau requested review from a team and brandyscarney as code owners March 25, 2024 19:36
thetaPC
thetaPC requested changes Mar 27, 2024
View reviewed changes

@thetaPC thetaPC left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verified that the issue has been fixed.

However, there are a few tests that are not related to the issue. Let's keep this PR specific to it and open a new PR that introduces these test changes.

@averyrousseau averyrousseau mentioned this pull request Mar 27, 2024
Merged
2 tasks
@github-actions github-actions Bot added package: angular @ionic/angular package package: vue @ionic/vue package package: react @ionic/react package labels Mar 27, 2024
@averyrousseau
averyrousseau marked this pull request as draft March 27, 2024 15:20
@averyrousseau
averyrousseau changed the base branch from main to toolbar-test-split March 27, 2024 15:21
@github-actions github-actions Bot removed package: angular @ionic/angular package package: vue @ionic/vue package package: react @ionic/react package labels Mar 27, 2024
@thetaPC

thetaPC commented Mar 27, 2024

Copy link
Copy Markdown
Contributor

@amandaejohnston Is this ready for review? It's marked as draft.

@averyrousseau

averyrousseau commented Mar 27, 2024

Copy link
Copy Markdown
Contributor Author

@thetaPC No, the screenshots haven't been updated yet here. Running the job causes them to be updated off of main instead of the toolbar-test-split branch (and I can't use the Docker stuff due to it not working for Windows yet), which causes all the screenshots to be generated new instead of this PR only showing the changed ones, so I'm hoping to get #29231 merged in so I can resolve things without having to debug all that.

github-merge-queue Bot pushed a commit that referenced this pull request Apr 1, 2024
@averyrousseau @Ionitron
test(toolbar): add dark theme to basic test (#29231)
2053bc7 
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. -->

The toolbar `basic` test only checks light theme. Dark theme coverage is
required to test against the bug fixed in
#29216.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

Dark theme coverage added. This required converting the test to
`page.setContent` instead of `page.goto`, so I went ahead and split the
captured toolbars into multiple tests, including some cleanup of the
test content. Since this included changes across many different tests,
even ones not strictly related to the bug, this work has been split into
a separate PR.

## 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. -->

---------

Co-authored-by: ionitron <hi@ionicframework.com>
Base automatically changed from toolbar-test-split to main April 1, 2024 18:28
@averyrousseau averyrousseau changed the title fix(button): activated outline button in toolbar no longer blends into background on dark mode fix(button): activated outline button in toolbar no longer blends into background on MD dark mode Apr 1, 2024
@averyrousseau

averyrousseau commented Apr 1, 2024

Copy link
Copy Markdown
Contributor Author

Well, suddenly glad I split the test changes out the way I did, because it made me realize I regressed the styling on iOS 😆

@averyrousseau
averyrousseau marked this pull request as ready for review April 1, 2024 19:21
@averyrousseau
averyrousseau requested a review from thetaPC April 1, 2024 19:21
@averyrousseau

averyrousseau commented Apr 1, 2024

Copy link
Copy Markdown
Contributor Author

@thetaPC This is ready for review again 👍

thetaPC
thetaPC approved these changes Apr 2, 2024
View reviewed changes

@thetaPC thetaPC left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, just a minor comment request

Comment thread core/src/components/button/button.md.scss
Comment thread core/src/components/button/button.ios.scss
thetaPC
thetaPC reviewed Apr 2, 2024
View reviewed changes
Comment thread core/src/components/button/button.ios.scss
@averyrousseau
averyrousseau added this pull request to the merge queue Apr 2, 2024
Merged via the queue into main with commit ee5da7a Apr 2, 2024
@averyrousseau
averyrousseau deleted the FW-5721 branch April 2, 2024 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thetaPC thetaPC thetaPC approved these changes
@brandyscarney brandyscarney Awaiting requested review from brandyscarney

Assignees

No one assigned

Labels

package: core @ionic/core package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@averyrousseau @thetaPC @Ionitron