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

[Misc] Footer: remove Join Us! highlight and all button links underline on hover #43

Merged
merged 4 commits into from
May 28, 2024

Conversation

nam-m
Copy link

@nam-m nam-m commented May 24, 2024

I removed the highlight className from Join Us! button and made all button text-decoration: none on hover in the footer

Copy link

vercel bot commented May 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
codeforbc-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 28, 2024 0:04am

Copy link
Collaborator

@CorreyL CorreyL left a comment

Choose a reason for hiding this comment

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

Did a quick test locally, and yeah hover over the links no longer renders an underline, and that doesn't seem to be a desired effect in the Figma, so this looks good to me!

I wanted to note that I committed 1bf8926 because I noticed originally the className of footer__button--highlight was removed in the first commit, but then checking the Figma design, it looks like the Join Us link should be distinctly rendered:

image

So my commit addresses this, I will let someone else review this PR since I have a code change in now too

@Unknown-0perator do you mind reviewing and approving this PR?

Copy link

@Unknown-0perator Unknown-0perator left a comment

Choose a reason for hiding this comment

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

@CorreyL, I agree, but I just want to confirm with @SherryW14. If it is not the hover state, it would be better to remove lines 73 to 75

@nam-m
Copy link
Author

nam-m commented May 25, 2024

Thanks @CorreyL and @Unknown-0perator for looking at it.
I felt the Join Us button shouldn't be different from other footer buttons, hence the change. Let's wait for Sherry to check it then!

Copy link

@Unknown-0perator Unknown-0perator left a comment

Choose a reason for hiding this comment

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

Please remove lines 73 to 75 (Sherry's comment) from #35 (comment); we don't need the hover state. Also, change "About" to "Get Engaged!"

Thanks!

text-decoration is set to none by default, so no need to specify the
hover selector
@CorreyL
Copy link
Collaborator

CorreyL commented May 28, 2024

Please remove lines 73 to 75 (Sherry's comment) from #35 (comment); we don't need the hover state.

Hover selector removed in e7f8496

Also, change "About" to "Get Engaged!"

Are we certain that About is meant to be turned into Get Engaged!? It seems to me that About should actually be replaced by Code of Conduct (when that page is ready)

image

I replaced Join Us! with Get Engaged! in 8c17a8e after double checking Sherry's comment in #35, but please let me know if it should actually link to /about

Copy link

@Unknown-0perator Unknown-0perator left a comment

Choose a reason for hiding this comment

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

Thank you @CorreyL, you are right it is Join Us not About

@CorreyL CorreyL merged commit 5753cca into main May 28, 2024
3 checks passed
@CorreyL CorreyL deleted the misc/fix-join-us-footer branch May 28, 2024 00:22
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.

3 participants