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

Move ICS Feed button from Ilios Calendar to Main Navigation #7811

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

michaelchadwick
Copy link
Contributor

Fixes ilios/ilios#4116.

The old functionality is to click on Calendar in the main navigation, click the ICS Feed button, and then click the "Copy My ICS Link" button in the flyout to copy the URL to the user's clipboard.

ilios-ics-pre

The new functionality is to click the ICS Feed button to copy the URL to the user's clipboard, directly from the main navigation, always available, and without the flyout.

ilios-ics-post

Tests for this are still being figured out, so this is a draft for now.

@michaelchadwick
Copy link
Contributor Author

I wasn't able to figure out how to test the new tooltip notification upon clicking the ICS Feed button, but I removed some testing code that was no longer relevant.

@michaelchadwick michaelchadwick marked this pull request as ready for review May 7, 2024 23:15
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 think putting this here was my idea, but now that it's here I don't love it. It feels like too prominent a location for this and doesn't balance particularly well with the other buttons. Some discussion needed for sure. Maybe in the profile menu?

The tooltip copy works, but I think we have some better options. Take a look at:

  1. The copy button on a virtual learning URL in an offering
  2. The copy button on the API key generator at the bottom of the user profile page

I didn't look too deeply at the code as is, think we need to work out some UX first.

@michaelchadwick
Copy link
Contributor Author

The API key copy notification is nice, yes, and could be used instead. As for the placement of the icon itself, I don't have a strong feeling one way or another. It looks like the user profile menu is pretty bare and could easily incorporate more functionality. However, would users know to look there for this?

@michaelchadwick
Copy link
Contributor Author

All righty: I changed the button to have an IliosTooltip on mouse hover, and the click success triggers a flash message instead.

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.

Can you have the tooltip pop out to the right instead of underneath when there is space? If this ends up being more than an hour's work never mind. I'd also like to see the button switch to a check after it is clicked along with the confirmation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants