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

💄 Unify focus ring color with brand theme #635

Merged
merged 8 commits into from
Feb 10, 2025

Conversation

JonathanDeLaCruzEncora
Copy link
Contributor

Summary

This PR changes the color of all buttons or focusable items, from the default color to the brand color

Related Issue

This changes solve issue #632

Changes

The code below was added to all the elements I was able to focus, 'auto' was recommended, but was still getting the default outline. After changing to 'solid' for the style, the changes were applied

:focus-visible {
  outline: 1px solid var(--primary-color);
}

Below are images of all the components that were changed

Screenshot 2025-01-31 at 3 43 35 p m

Screenshot 2025-01-31 at 3 45 11 p m

Buttons from the toolbar were focusable with drawers closed, when clicking on them and then hitting spacebar. So its a rare case, but they are focusable.

Screenshot 2025-01-31 at 3 44 50 p m

The same with mobile tools.

Screenshot 2025-01-31 at 3 46 14 p m

@JonathanDeLaCruzEncora JonathanDeLaCruzEncora requested a review from a team as a code owner January 31, 2025 23:05
@JonathanDeLaCruzEncora JonathanDeLaCruzEncora requested review from hoshinotsuyoshi, FunamaYukina, junkisai, MH4GF and sasamuku and removed request for a team January 31, 2025 23:05
Copy link

changeset-bot bot commented Jan 31, 2025

⚠️ No Changeset found

Latest commit: c5f0d44

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@MH4GF
Copy link
Member

MH4GF commented Feb 3, 2025

The Vercel deployment doesn't work with PR from fork. I'll fix it later!

@MH4GF
Copy link
Member

MH4GF commented Feb 3, 2025

@JonathanDeLaCruzEncora Can you also adjust each button and link in the header?

Screenshot 2025-02-03 at 11 41 32 Screenshot 2025-02-03 at 11 42 52

@MH4GF
Copy link
Member

MH4GF commented Feb 3, 2025

@JonathanDeLaCruzEncora
Sorry to add this, could you please make these two buttons square like the zoom button on the left? Please match border-radius too.

Pasted_Image_2025_02_03__11_44

@JonathanDeLaCruzEncora
Copy link
Contributor Author

Buttons in header updated, as well as the mobile open/close button and other buttons like the close button in the toolbar which I didn't change previously.

All the buttons that had squared outlines now are rounded, including the view/hide button in the side menu (I also added the brand color to its outline)

If there are any changes feel free to let me know 😄

@MH4GF
Copy link
Member

MH4GF commented Feb 5, 2025

@JonathanDeLaCruzEncora Oh, sorry, please resolve conflict?

Copy link
Member

@MH4GF MH4GF left a comment

Choose a reason for hiding this comment

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

LGTM😄 Thanks for adjusting!
Just resolve the conflicts and we'll merge!

@MH4GF
Copy link
Member

MH4GF commented Feb 6, 2025

@JonathanDeLaCruzEncora Just correct the format at the end!

Press here and I'll notice you right away!
Screenshot 2025-02-06 at 18 50 08

@MH4GF MH4GF enabled auto-merge February 6, 2025 23:37
@JonathanDeLaCruzEncora JonathanDeLaCruzEncora had a problem deploying to preview - @liam-hq/erd-sample February 6, 2025 23:37 — with GitHub Actions Failure
@MH4GF
Copy link
Member

MH4GF commented Feb 7, 2025

@JonathanDeLaCruzEncora CI is down. pnpm fmt will fix it, please fix it.

auto-merge was automatically disabled February 7, 2025 16:13

Head branch was pushed to by a user without write access

@JonathanDeLaCruzEncora
Copy link
Contributor Author

@MH4GF Thank you so much for that, it should be fine now, wouldn't have done it without your help

@MH4GF MH4GF added this pull request to the merge queue Feb 10, 2025
Merged via the queue into liam-hq:main with commit f7c727b Feb 10, 2025
9 of 11 checks passed
@MH4GF
Copy link
Member

MH4GF commented Feb 10, 2025

@JonathanDeLaCruzEncora Thanks for your perseverance!

@JonathanDeLaCruzEncora JonathanDeLaCruzEncora deleted the feat/unify-focus-ring branch February 19, 2025 21:43
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