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

Add icon to header to link to User Guide #7821

Merged

Conversation

michaelchadwick
Copy link
Contributor

Fixes ilios/ilios#3759.

Created new UserGuideLink component that displays an icon to click that opens the Ilios User Guide in a new window. It has translations for the <svg><title> element, and a simple component test.

@dartajax
Copy link
Member

neat - any chance of making the icon a little bit bigger? 20% or so - that's my $.02, which could get vetoed - looks good though all in all

@michaelchadwick
Copy link
Contributor Author

@dartajax No problem on the size change. I also noticed I need to adjust it for smaller screens, so I'll fix that, too.

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.

Long time coming this one. I'm not in love with the icon and think this probably belongs in the footer instead of the header but will follow @dartajax's lead here. Otherwise just needs a small adjustment to the tests.

assert.dom('.user-guide-link svg.fa-circle-question').exists();
assert.dom('.user-guide-link svg.fa-circle-question title').hasText('Ilios User Guide');

this.owner.lookup('service:intl').setLocale('es');
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to test the translations, but if you want to follow the pattern in https://ember-intl.github.io/ember-intl/docs/guide/testing to set the locale or test the untranslated output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated tests to be more in line with convention.

@jrjohnson
Copy link
Member

Might be the "regular" I don't love. We have a license for everything.

@michaelchadwick
Copy link
Contributor Author

Oddly, though, the <FaIcon> component had no hook to use fa-regular, only fa-solid and fa-brands, so that is why I made the change to the component in this PR.

Regardless, I ended up using the fa-solid version, anyway, but just changed the color of the svg to be white.

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.

Pedantic CSS changes requested. Sorry.

@use "../mixins" as m;

.user-guide-link {
@include m.header-menu;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is quite a header menu, too many styles in there don't apply here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the mixin and used a simpler style to get the same effect.


@include cm.for-phone-only {
/* stylelint-disable property-disallowed-list */
font-size: 3.5vw;
Copy link
Member

Choose a reason for hiding this comment

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

I don't love using this disallowed property, Could this be done by controlling width and height of the icon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this disallowed property and used height/width instead.

align-items: center;
color: c.$white;
display: flex;
height: 1.85em;
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this can be done as a percentage of the container. Not sure, just feels a little arbitrary and might break if we accidentally slightly change the header height.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to use pixels instead. Using percentage of the container did not seem to work.

@michaelchadwick michaelchadwick linked an issue Jul 2, 2024 that may be closed by this pull request
@jrjohnson
Copy link
Member

Code looks good, assigning to @dartajax for visual review and merging.

@stopfstedt stopfstedt removed their request for review July 18, 2024 18:59
Copy link
Member

@dartajax dartajax left a comment

Choose a reason for hiding this comment

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

looks good to me - percy isn't happy though

@michaelchadwick michaelchadwick force-pushed the frontend-3759-add-guide-icon-to-header branch from 2fb40ba to 2e3a0b0 Compare July 18, 2024 20:31
@jrjohnson
Copy link
Member

Percy just needs approval of the changes @dartajax, because this header is on every page it's a lot of changes, but still worth checking through I think.

@dartajax dartajax merged commit e5f841e into ilios:master Jul 19, 2024
36 checks passed
@michaelchadwick michaelchadwick deleted the frontend-3759-add-guide-icon-to-header branch July 21, 2024 03:15
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.

add "help" link to header/footer/menu Consider Adding Link to User Guide in Ilios Application
3 participants