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

improve User Guide icon #7998

Conversation

michaelchadwick
Copy link
Contributor

Fixes ilios/ilios#5609

My attempt to address all the issues:

  • Color scheme made consistent
  • Icon shape made consistent
  • Question mark uses the same base font as all other Fontawesome icons, so...maybe not fixable?
  • Large screen icon alignment fixed

To do this, I had to go from using an <a> (which I chose since the icon takes you somewhere instead of just doing something) to using a <button> (which all the other elements use, giving it a lot of the same existing CSS properties) and adding a component class to model the "click this to open a new window/tab" action

Copy link
Member

@jrjohnson jrjohnson left a comment

Choose a reason for hiding this comment

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

I agree this should be a link. I'd rather keep the semantically correct HTML, but if that isn't possible I'll accept it. Take a look at the ilios-link-button mixin though, it may help.

@michaelchadwick
Copy link
Contributor Author

I'll take another crack at this to keep the semantic <a>-ness of it, but I wanted to get it out there for others to look at for visual and functional reasons.

@michaelchadwick
Copy link
Contributor Author

Changed back from <button> to <a> so semantic again. Had to fudge some CSS here and there, but I think it is both a bit smaller and a more distant from the other icons.

@stopfstedt stopfstedt requested a review from jrjohnson July 26, 2024 16:43
jrjohnson
jrjohnson previously approved these changes Jul 26, 2024
Copy link
Member

@jrjohnson jrjohnson left a comment

Choose a reason for hiding this comment

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

Excellent. Looks great to me.

@saschaben
Copy link
Member

please if we are able: align center vertically, rather than align-bottom viz the other icons; add 2px padding on right, since when vertical scroll appears, it overlays the right edge of the icon.

otherwise, all A-OK

@michaelchadwick
Copy link
Contributor Author

@saschaben Thanks for the feedback. Will hammer it out some more soon.

@michaelchadwick michaelchadwick force-pushed the frontend-5609-user-guide-icon-improvements branch from 6cfeeb1 to 120847c Compare July 26, 2024 21:38
@michaelchadwick
Copy link
Contributor Author

@saschaben Made some adjustments. Let me know if this works better!

@michaelchadwick michaelchadwick force-pushed the frontend-5609-user-guide-icon-improvements branch from 120847c to 42cbd5d Compare August 7, 2024 22:28
Copy link
Member

@jrjohnson jrjohnson left a comment

Choose a reason for hiding this comment

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

Excellent

@michaelchadwick michaelchadwick merged commit 151ed26 into ilios:master Aug 8, 2024
39 of 40 checks passed
@michaelchadwick michaelchadwick deleted the frontend-5609-user-guide-icon-improvements branch September 13, 2024 23:06
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.

User Guide icon could use some improvement
3 participants