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

1127-nav-hoverContrast change hover to red #1148

Merged
merged 4 commits into from
May 6, 2022
Merged

Conversation

ezhou0
Copy link
Member

@ezhou0 ezhou0 commented Oct 28, 2021

Fixes #1127

  • Up to date with dev branch
  • Branch name follows guidelines
  • All PR Status checks are successful
  • Peer reviewed and approved

Any questions? See the getting started guide

@EchoProject EchoProject marked this pull request as draft November 5, 2021 02:01
@KristenDLR KristenDLR marked this pull request as ready for review November 12, 2021 00:43
Copy link

@KristenDLR KristenDLR left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@EchoProject EchoProject requested a review from roz0n November 19, 2021 02:54
@EchoProject
Copy link
Contributor

EchoProject commented Nov 19, 2021

@ezhou0 Please provide a screenshot of the before and after views.

@ezhou0
Copy link
Member Author

ezhou0 commented Dec 7, 2021

Screen Shot 2021-12-06 at 10 58 23 PM

Screen Shot 2021-12-06 at 10 58 34 PM

Screen Shot 2021-12-06 at 10 58 57 PM

Properly shows underline under the navlink of the active page.
DOES NOT show underline onclick for reports because it is a dropdown not a navlink.
REMOVED red hover and went back to default MUI button hover

Copy link
Member

@adamkendis adamkendis left a comment

Choose a reason for hiding this comment

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

LGTM!

@adamkendis
Copy link
Member

I'll fix the build pipeline and merge this in over the weekend unless you beat me to it.

@nichhk
Copy link
Member

nichhk commented May 5, 2022

My best guess ATM for the build failure is that GitHub Actions upgraded the default Node version to 16, right around the time that the build failures began.

This guess is corroborated by the fact that npm run setup (from client setup) failed when I was using Node 18, but succeeds when I use Node 12. I knew I should try using Node 12 because the first error I got when running npm run setup was:

npm ERR! code 1 npm ERR! path /Users/nich/hackforla/311-data/client/node_modules/node-sass

which points out node-sass; I checked the package.json file, and it requires a fairly old version of node-sass (see mapping from node-sass to Node versions here).

Every Action should specify to use Node 12 using setup-node. I will check if this works and create a new PR if so. We can also explore upgrading node-sass, but the former solution seems like the smallest one.

Regardless of whether that's the fix, we should make it plainly obvious what version of Node we expect to run on. We can do that by stating it in the package.json file (see stackoverflow question).

@nichhk
Copy link
Member

nichhk commented May 6, 2022

#1183 merged into dev, and I merged dev back into this branch. The tests pass now, so I will merge this PR into dev.

@nichhk nichhk merged commit 11f8b95 into dev May 6, 2022
@nichhk nichhk deleted the 1127-nav-hoverContrast branch May 6, 2022 02:36
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.

Navigation hover contrast
5 participants