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: remove redundant margin property #61

Merged
merged 1 commit into from
Jun 18, 2024
Merged

fix: remove redundant margin property #61

merged 1 commit into from
Jun 18, 2024

Conversation

Kaciras
Copy link
Contributor

@Kaciras Kaciras commented Jun 17, 2024

icons margin

The close icon has an extra padding attribute than the icon on the left, causing them to be at different distances from the border.

This PR increases margin of the left icon, makes the space on both sides equal in width.

@smastrom
Copy link
Owner

Hi @Kaciras, thank you for your PR.

Actually both the icon and the close button have the same horizontal distance (margin) from the container. I consider the padding of the close button being part of the button shape which can be noticed once hovered:

Notivue notification

Notivue notification

You can see in above screenshots (but also yours) that the margin is clearly the same so there's nothing to "fix".

The "extra" padding you mentioned is something that must be there otherwise it would become difficult clicking/tapping it if too small. On the other hand, this doesn't mean that the space around the status icon (or the icon itself) should be equal or greater than the close button.

That said, I totally get your PR but I think this is more of a subjective preference than something that should be merged and then applied to everyone using this package.

At the end of the day if you think this is wrong you can always "fix" it on your own with one line of CSS. I hope this is clear and thank your for your PR.

I have noticed that you removed a useless line of CSS so I'm happy to merge this if you revert the icon class.

@Kaciras
Copy link
Contributor Author

Kaciras commented Jun 18, 2024

Thanks for your review. I have updated the commit.

@smastrom smastrom changed the title Fix inconsistent icon margin. fix: remove redundant margin property Jun 18, 2024
@smastrom smastrom merged commit bd49fb7 into smastrom:main Jun 18, 2024
1 check passed
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.

2 participants