-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 clear button hover effect #3278
base: develop
Are you sure you want to change the base?
Fix clear button hover effect #3278
Conversation
Thanks for working on this, I think this is a great start! One note I might add is that I personally don't prefer how the pink background enlarges on hover, and feel that it might be distracting and really noticeable for users, especially since this type of animation is not implemented in other parts of the editor. However, I do really like the smooth transition that's been added! I'm wondering maybe in this PR, we could keep the transition without enlarging the pink background for now. The more animated transitions could be added in a separate one in the future as a set of changes across the editor so that it looks more visually cohesive? |
@raclim I have done the changes now it's not scaling i have just hover effect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added in a few notes on some of the additional changes that would be needed here! Here's an idea of what they might look like in _console.scss
:
.preview-console__clear {
@include themify() {
@extend %link;
color: getThemifyVariable('secondary-text-color');
&:hover {
transition: color 0.3s ease,
box-shadow 0.3s ease,
transform 0.3s ease,
border-color 0.3s ease;
background: add themify variable;
color: add themify variable;
border-radius: 2px;
}
}
background: transparent;
border: none;
margin-right: #{math.div(4, $base-font-size)}rem;
.preview-console--collapsed & {
display: none;
}
}
Let me know if you have any questions about any of this!
&:hover { | ||
color: getThemifyVariable('heavy-text-color'); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update on this! I'm reviewing the code more closely and noticed a few areas that would need more adjustments.
First, I see that you removed the &:hover
on line 91, presumably to address the hover styles instead within .light .preview-console__clear:hover
on lines 105 - 108. However, the &:hover
was already handling the hover styling effectively, so the additional lines on 105-108 don't seem necessary. Additionally, those lines only target the .light
theme, which creates a gap for the other themes, dark
and contrast
.
It seems like these lines were adapted from the attached issue, but that code was intended as a mockup rather than a fully implemented solution. To ensure consistency across all themes, I'd suggest continuing to manage the hover effect styling within &:hover
, and to use the themifyVariables
to assign color values rather than hardcoding hex codes.
@@ -129,13 +129,13 @@ | |||
"jest-styled-components": "^7.1.1", | |||
"lint-staged": "^10.5.4", | |||
"mini-css-extract-plugin": "^1.6.2", | |||
"msw": "^0.35.0", | |||
"nodemon": "^2.0.22", | |||
"msw": "^2.6.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we want to avoid any changes to package.json
and package-lock.json
unless absolutely necessary. I don't believe those changes are needed here, so it would be best to remove them from this PR!
I also want to add to please ensure that your PR passes the linting and tests before signaling that it's ready for another review! |
I think it might have to do with the Based on the commit message though, it seems like that you weren't able to load the codebase without those specific changes? |
Fixes #2592
Changes: 1.Updated the background color and added a smooth transition for the hover effect of the "Clear" button.
2.Introduced subtle animations for a modern and engaging interaction
I have verified that this pull request:
p5.js.Web.Editor.-.Google.Chrome.2024-11-17.13-11-06.mp4
npm run lint
)npm run test
)develop
branch.Fixes #123