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

feat: revamp recipes list page #1199

Merged
merged 4 commits into from
Jun 13, 2024

Conversation

axel7083
Copy link
Contributor

@axel7083 axel7083 commented Jun 11, 2024

What does this PR do?

Following one of the mockup in #1126, improving the recipes card.

Notable change

  • Adding an icon showing the status (cloned or not) similar to the one in extension catalog
  • Adding the path in the card

Screenshot / video of UI

image

image

image

What issues does this PR fix or reference?

Fixes partially #1126

How to test this PR?

  • unit tests has been provided

axel7083 added 2 commits June 12, 2024 15:52
Signed-off-by: axel7083 <[email protected]>
@axel7083 axel7083 force-pushed the feature/revamp-recipes-list branch from 14faeac to 0c99854 Compare June 12, 2024 13:53
Signed-off-by: axel7083 <[email protected]>
@axel7083 axel7083 marked this pull request as ready for review June 12, 2024 14:15
@axel7083 axel7083 requested review from benoitf and a team as code owners June 12, 2024 14:15
@axel7083 axel7083 requested review from lstocchi, jeffmaury and feloy June 12, 2024 14:15
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

for the path inside the card I don't know if we should truncate the middle of the path so we see the beginning and the end of the path but it can also be a bad idea !

@axel7083
Copy link
Contributor Author

axel7083 commented Jun 12, 2024

for the path inside the card I don't know if we should truncate the middle of the path so we see the beginning and the end of the path but it can also be a bad idea !

We will probably iterate a bit on this with Emma when she will be back ! I keep your remark in mind, I also feel a bit weird with the current path.

I cannot put a tooltip, because there is like overflow-hidden everywhere on the page...

@jeffmaury
Copy link
Contributor

for the path inside the card I don't know if we should truncate the middle of the path so we see the beginning and the end of the path but it can also be a bad idea !

Yes can't we have the full path as tooltip ?

Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

LGTM comment wise

@feloy
Copy link
Contributor

feloy commented Jun 13, 2024

I find the recipe status button/indicator disturbing. It's ok when the recipe is downloadable, but when it is downloaded, the button is still active and does nothing. Also, a tooltip would help understand the meaning

Signed-off-by: axel7083 <[email protected]>
@axel7083
Copy link
Contributor Author

axel7083 commented Jun 13, 2024

Following @feloy advise, I revised the design of the icon.

  • First I noticed I was using chevron down instead of circle check which were looking weird
  • I removed the border and hover effect when the recipe cloned
  • I also added a tooltip on the button to improve understanding
demo-revamp-recipes-page.mp4

Yes can't we have the full path as tooltip ?

I was able to make it works, as the path is often too big, it go offscreen, I tried to play with the tooltip position (left, right etc.) but depending on which card, it get always cut offscreen...

@axel7083
Copy link
Contributor Author

axel7083 commented Jun 13, 2024

⚠️ NOT INCLUDED IN THIS PR

I had a little fun with finding a nice way of allowing the user to see the full path, and I though: what if the text automatically scroll when hovering

text-hover-scroll.mp4

if this idea please some people, I could make another PR to include it, what do you think @jeffmaury @benoitf @deboer-tim

I can control the speed of the animation, I am also thinking we could add cursor-copy and make it copy when the user click on it

@jeffmaury
Copy link
Contributor

⚠️ NOT INCLUDED IN THIS PR

I had a little fun with finding a nice way of allowing the user to see the full path, and I though: what if the text automatically scroll when hovering
text-hover-scroll.mp4

if this idea please some people, I could make another PR to include it, what do you think @jeffmaury @benoitf @deboer-tim

I can control the speed of the animation, I am also thinking we could add cursor-copy and make it copy when the user click on it

Seems to me that it is more common to have a UI glitch (tooltip) that shows the full path when a specific UI action is taken (hover,...). Here the con that I see is that the full path is not displayed entirely but that's still an improvement

@feloy
Copy link
Contributor

feloy commented Jun 13, 2024

⚠️ NOT INCLUDED IN THIS PR
I had a little fun with finding a nice way of allowing the user to see the full path, and I though: what if the text automatically scroll when hovering
text-hover-scroll.mp4
if this idea please some people, I could make another PR to include it, what do you think @jeffmaury @benoitf @deboer-tim

I can control the speed of the animation, I am also thinking we could add cursor-copy and make it copy when the user click on it

Seems to me that it is more common to have a UI glitch (tooltip) that shows the full path when a specific UI action is taken (hover,...). Here the con that I see is that the full path is not displayed entirely but that's still an improvement

Also, one important thing for the user I think is to be able to copy the path string. If the path is not displayed completely, the copy is not effective. The tooltip won't help either, as its content cannot be copied. Should we add a copy button? (same as @axel7083 added in his initial message I guess)

@axel7083
Copy link
Contributor Author

axel7083 commented Jun 13, 2024

Merging for now this PR and I will open an issue for follow up here #1214

@axel7083 axel7083 merged commit d0732dc into containers:main Jun 13, 2024
4 checks passed
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.

4 participants