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 background color issue on hover for sketch name by username #2676 #3408

Conversation

Harshit-7373
Copy link
Contributor

@Harshit-7373 Harshit-7373 commented Mar 19, 2025

Fixes #2676

Changes:

  1. I have updated the classname and the SCSS file to resolve this issue.

Here is a video with the required changes :

Screen.Recording.2025-03-19.at.10.56.19.PM.mov

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

@@ -61,7 +61,6 @@ const Toolbar = (props) => {
</button>
<button
className={playButtonClass}
id="play-sketch"
Copy link
Contributor

@Jatin24062005 Jatin24062005 Mar 28, 2025

Choose a reason for hiding this comment

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

Why you removed this???

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, in the context of this PR, I think this should not be removed!

color: $p5-light-pink;

// If the logo color is yellow, override the hover color
@if (getThemifyVariable('logo-color') == $yellow) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think one issue here is similar to the one I noted in your other PR, where we can use just the getThemifyVariable utility function directly rather than the conditional.

@raclim
Copy link
Collaborator

raclim commented Mar 28, 2025

Thanks so much for looking into this!

We actually have a PR that addresses this issue, submitted by the same contributor who originally opened it. At the time, I closed it because I felt we needed a clearer, unified design direction for how links should appear before moving forward with any changes.

That said, the updates in that PR are quite similar to what you've proposed here, so I think it makes sense to move ahead with the original one to keep things consistent and acknowledge the initial contribution. With that in mind, I'm going to go ahead and close this for now.

@raclim raclim closed this Mar 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants