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

Consider hover indictation radius in toolbar #19018

Closed
karmatosed opened this issue Dec 9, 2019 · 4 comments · Fixed by #19344
Closed

Consider hover indictation radius in toolbar #19018

karmatosed opened this issue Dec 9, 2019 · 4 comments · Fixed by #19344
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later

Comments

@karmatosed
Copy link
Member

karmatosed commented Dec 9, 2019

The hover on the toolbar has a radius:

image

You can see it clearly here:

image

If the border-radius is removed it looks like this, which feels more fitting for now:

image

It's worth noting this uses a shadow to get hover effect and seems related to a component, so a PR might need some deeper digging.

@karmatosed karmatosed added the Needs Design Feedback Needs general design feedback. label Dec 9, 2019
@shaunandrews
Copy link
Contributor

I don't think removing the border-radius is the right solution. That radius is applied to all buttons and icon-buttons throughout the editor, including in the top toolbar. I think the better solution might be to add 2px of space around the icon-buttons in the toolbar, bumping up the size of the toolbar to 40px.

Here's the current (36px) space, vs the proposed (40px) space:

image

@phpbits
Copy link
Contributor

phpbits commented Dec 10, 2019

I don't think removing the border-radius is the right solution. That radius is applied to all buttons and icon-buttons throughout the editor, including in the top toolbar. I think the better solution might be to add 2px of space around the icon-buttons in the toolbar, bumping up the size of the toolbar to 40px.

+1, adding margin will make it look better. Here's what I'm using on EditorsKit plugin for toolbar button with text content :

Consider hover indictation radius in toolbar

@karmatosed karmatosed added Good First Issue An issue that's suitable for someone looking to contribute for the first time [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later and removed Needs Design Feedback Needs general design feedback. labels Dec 18, 2019
@karmatosed
Copy link
Member Author

Marking as good first issue as making a PR to add margin could be a good starting place.

@Addison-Stavlo
Copy link
Contributor

Addison-Stavlo commented Jan 16, 2020

I have opened a PR (noted above) that does as @shaunandrews suggested.

I have a question there (with screenshots) regarding weather or not we also want this margin on the block-mover part of the toolbar as well.

@youknowriad youknowriad mentioned this issue Jan 17, 2020
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants