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

Try moving the Modal close button over to the right #16883

Merged
merged 1 commit into from
Aug 5, 2019

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Aug 2, 2019

Currently, the modal close button is in right-aligned to the modal content. But because the button has no border by default, the x icon button appears misaligned:

current-default

When hovering over the button, the borders make the intended alignment more clear:

current-hover


This PR tries shifting the button over to the right by 8px to offset the button padding. This properly aligns the x icon with the right side of the content below it:

new-default

On hover, the borders appear, and the intentional misalignment becomes clear, but I don't think this is much of an issue. I'd rather that this appeared misaligned in the hover state than in its default state.

new-hover

@davewhitley
Copy link
Contributor

It would help if the X icon was bigger inside of its 20px container, but this looks like a good quick fix.

@kjellr
Copy link
Contributor Author

kjellr commented Aug 2, 2019

It would help if the X icon was bigger inside of its 20px container, but this looks like a good quick fix.

Something like this?

x

That's a 24px icon inside of the same size container, with the position shifted just 4px to the right so things line up.

@karmatosed
Copy link
Member

I think if we do increase we need to also increase other icons, which is something to do but feels outside the small fix of this issue. Beyond that, works well for me.

@karmatosed karmatosed self-requested a review August 5, 2019 10:30
Copy link
Member

@karmatosed karmatosed left a comment

Choose a reason for hiding this comment

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

The hover state feels a little unusual with border now but I think that's something to explore in another issue maybe. It certainly is better aligned with this PR on non-hover.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants