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 interaction styles for the navigation bar and the footer links #642

Merged

Conversation

chosww
Copy link
Contributor

@chosww chosww commented Jun 26, 2023

  • This pull request has been tested by running npm run test without errors
  • This pull request has been built by running npm run build without errors
  • This isn't a duplicate of an existing pull request

Description

I noticed that focus and hover styles for the items on navigation bar is broken in different themes. I did some investigation and found out that UIO applies different border colours for different themes and we didn't set any border on the navigation bar items, and that caused no focus and hover styles on them.

An approach I took for the nav bar items are to set transparent border on them all the time by overriding UIO styles, and on focus and hover states, apply correct UIO styles. This way, we can utilize UIO. A potential approach would be not to use UIO, and use outline for the focus and hover styles, but that would be a lot of custom css styles we may maintain in the future as UIO evolves.

I also noticed that focus and hover styles for items on the footer are off, so I made some adjustment to them.

Steps to test

  1. Check hover styles on live site
  2. Compare the styles with the preview of this PR

Expected behavior:

Nav bar items should have proper interaction styles in different themes changed by UIO.

@netlify
Copy link

netlify bot commented Jun 26, 2023

Deploy Preview for idrc ready!

Name Link
🔨 Latest commit 3b1f5f5
🔍 Latest deploy log https://app.netlify.com/sites/idrc/deploys/64a427ca92cbd80008e5f63a
😎 Deploy Preview https://deploy-preview-642--idrc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@greatislander greatislander left a comment

Choose a reason for hiding this comment

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

There are a few issues with this— let's discuss when you have time, @chosww.

src/assets/styles/base/_base.scss Outdated Show resolved Hide resolved
src/assets/styles/components/_nav.scss Outdated Show resolved Hide resolved
@greatislander greatislander added the bug Something isn't working label Jun 27, 2023
Copy link
Member

@greatislander greatislander left a comment

Choose a reason for hiding this comment

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

One change that I can see— thanks for all your work on this, @chosww!

src/assets/styles/components/_nav.scss Outdated Show resolved Hide resolved
Copy link
Member

@jhung jhung left a comment

Choose a reason for hiding this comment

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

@chosww Thanks for fixing the hover and focus styling!

I noticed that px units are being used in the SCSS. Previously we have tried to use relative values (rem, vw, vh, etc.) where possible. Can the PR be updated to use rems etc?

@greatislander greatislander requested a review from jhung July 4, 2023 15:14
@greatislander greatislander merged commit 28a4aca into inclusive-design:dev Jul 4, 2023
7 checks passed
@greatislander greatislander added this to the 2.0.0 milestone Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants