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: update appearance/index.ts to apply the accent color on tabs #1967

Merged
merged 2 commits into from
Nov 12, 2024

Conversation

DenysMb
Copy link
Contributor

@DenysMb DenysMb commented Nov 12, 2024

Pre-flight Checklist

Please ensure you've completed all of the following.

Description of Change

After this previous PR #1944, as stated by @wvds here #1944 (comment), the accent color was not being updated in tabs. This happened because, with the change from border to box-shadow, I forgot to update the appearance/index.ts, where we do this change. It was still changing the border-color, but since there is no more border, it was doing nothing. I just needed to update the code there to use the box-shadow.

Motivation and Context

This is a bug.

Screenshots

image
image

Checklist

  • My pull request is properly named
  • The changes respect the code style of the project (pnpm prepare-code)
  • pnpm test passes
  • I tested/previewed my changes locally

Release Notes

Fix the problem that was causing the accent colors to not be applied to tabs indicator

@vraravam
Copy link
Contributor

vraravam commented Nov 12, 2024

@DenysMb - Thanks for creating the PR so quickly, but please create PRs using the latest commit from the develop branch. Otherwise, all testing (especially visual changes) will not be verifiable in the correct manner.
And, of course, the CI runs unnecessarily.

@vraravam vraravam force-pushed the fix/apply-accent-colors-on-tabs branch from 69eba8f to 4704ee6 Compare November 12, 2024 11:55
@DenysMb
Copy link
Contributor Author

DenysMb commented Nov 12, 2024

@vraravam Yeah, sorry. I forgot to fetch the changes before I made the changes.

@vraravam
Copy link
Contributor

@vraravam Yeah, sorry. I forgot to fetch the changes before I made the changes.

Please retest with the latest version since I have rebased from the latest develop. Once you do and confirm that the fix works, we can merge (after the CI run also completes successfully)

@DenysMb
Copy link
Contributor Author

DenysMb commented Nov 12, 2024

@vraravam Everything working fine here after the rebase.

Copy link
Contributor

@vraravam vraravam left a comment

Choose a reason for hiding this comment

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

not tested by myself. OP says its working.

@vraravam vraravam merged commit 58fc88a into ferdium:develop Nov 12, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants