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

Fixes #34755 - Fix text/icon alignment on host status card #9183

Merged
merged 1 commit into from Apr 11, 2022

Conversation

jeremylenz
Copy link
Contributor

This is an attempt to improve the alignment and spacing of the text and icons on the AggregateStatusCard.

Before:
host_status_bad_alignment

After:
host_status_new_alignment

let me know your thoughts

@jeremylenz jeremylenz changed the title Fixes #37455 - Fix text/icon alignment on host status card Fixes #34755 - Fix text/icon alignment on host status card Apr 8, 2022
Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Just out of curiosity: do we have to use inline styling here?.. Couldn't it be extracted to the corresponding scss file?

@jeremylenz
Copy link
Contributor Author

Couldn't it be extracted to the corresponding scss file?

I played around with it for a bit, but I'm realizing would be quite tedious to do it that way. I would have to add classes to several elements that don't have them, and modify others, and also come up with a CSS solution for the props override of the style I do for <StatusIcon> - it's possible, but I just don't see a reason to. Would you be okay keeping it as-is for this reason?

Copy link
Member

@Ron-Lavi Ron-Lavi left a comment

Choose a reason for hiding this comment

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

Overall looks great, there's a small issue when hovering the icons:

Screenshot (75)

@jeremylenz
Copy link
Contributor Author

@Ron-Lavi good catch! The elements inside the <a> were all underlined, but since I moved the icon into alignment with position: relative its underline also moved.

Updated to no longer underline the icon on hover.

Copy link
Member

@Ron-Lavi Ron-Lavi left a comment

Choose a reason for hiding this comment

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

Thanks @jeremylenz!

@Ron-Lavi Ron-Lavi merged commit 57af9fe into theforeman:develop Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants