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 clear button hover effect #3278

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

ujjwaldubey1
Copy link

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
  • 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

@raclim
Copy link
Collaborator

raclim commented Nov 18, 2024

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?

@ujjwaldubey1
Copy link
Author

@raclim I have done the changes now it's not scaling i have just hover effect

Copy link
Collaborator

@raclim raclim left a 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');
}

Copy link
Collaborator

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",
Copy link
Collaborator

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!

@raclim
Copy link
Collaborator

raclim commented Nov 26, 2024

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!

@ujjwaldubey1
Copy link
Author

image
this error is showing

@raclim
Copy link
Collaborator

raclim commented Nov 27, 2024

I think it might have to do with the package.json file changes, which seem to be isolated into this commit. I think finding a way to remove that specific commit would potentially resolve the file differences.

Based on the commit message though, it seems like that you weren't able to load the codebase without those specific changes?

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.

Insufficient hover effect on console Clear button
2 participants