Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix carousel when color-mode-type is media-query #38325

Closed
wants to merge 1 commit into from

Conversation

malberts
Copy link

@malberts malberts commented Mar 25, 2023

Description

Bug: #38322
This adds a trivial condition to apply #38209 only when @color-mode-type=dark.

Motivation & Context

It fixes #38322.

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Related issues

@malberts malberts requested a review from a team as a code owner March 25, 2023 07:06
@malberts
Copy link
Author

malberts commented Mar 25, 2023

What would be the best way to add an automated test case for this? I had a quick look but couldn't immediately figure it out. I don't think the Jasmine tests can do this, because it's not actually compiling the whole SCSS. Basically we want to compile the SCSS with a different set of variables. If there's a good example and I missed it, let me know and I'll follow up.

@julien-deramond
Copy link
Member

Yep I agree for the automated test, I've suggested something in #38326. We'll try to look at your fix asap. Thanks for the PR @malberts.

@malberts
Copy link
Author

Thanks @julien-deramond, I understand how this can be done now.

npm run css-test passes locally with that test and my change:
Screenshot_20230325_113558

@mdo
Copy link
Member

mdo commented Mar 25, 2023

I didn't see this PR before creating mine, but wanted to share that I prefer the combined selector output from #38328 over this option. I'll go ahead and close this out and we'll plan to ship that one.

@mdo mdo closed this Mar 25, 2023
@malberts malberts deleted the carousel-dark branch March 26, 2023 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build error in carousel.scss when using $color-mode-type: media-query
3 participants