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.

Sign up for GitHub

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

Jump to bottom

feat(atomic): add result action bar on hover #3002

Merged
merged 15 commits into from
Jul 10, 2023
Merged

feat(atomic): add result action bar on hover #3002

merged 15 commits into from
Jul 10, 2023

Conversation

jelmedini
Copy link
Contributor

@jelmedini jelmedini commented Jun 26, 2023 •
edited
Loading

SVCC-2942

Add result action bar component for the insight panel use case
Acceptance Criteria:

  • The result actions button bar should show when hovering over a result
  • Hovering over the buttons should display a tooltip with the name of the result action
  • The result action should require an icon and a label
  • Hovering over a result should change the background color.

When Hover :
Screenshot 2023-06-28 at 3 33 52 PM

Video Demo:

Screen.Recording.2023-07-05.at.4.41.33.PM.mov
Screenshot 2023-06-26 at 7 57 16 PM

@jelmedini jelmedini requested review from a team as code owners June 26, 2023 20:24
@jelmedini jelmedini requested review from olamothe, louis-bompart and a user June 26, 2023 20:24
@github-actions
Copy link

github-actions bot commented Jun 26, 2023 •
edited
Loading

Dependency Review

✅ No vulnerabilities or license issues found.

Scanned Manifest Files

@nathanlb
Copy link
Contributor

nathanlb commented Jun 26, 2023 •
edited
Loading

Haven't checked the code yet but the video looks real good! Couple styling comments:

  1. Nice tooltip!
  2. Could we have the action bar aligned completely to the right? Looks like there's an empty space there.
  3. Maybe we should use a shade of grey that is slightly different from the default badge color so that we can still see it?
248925819-3906d18e-98bd-4b84-ad46-39a7c4a36983

@github-actions
Copy link

github-actions bot commented Jun 26, 2023 •
edited
Loading

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

Bundle Size

File Old (kb) New (kb) Change (%)
case-assist 179.7 179.7 0
search 336.7 336.7 0
insight 291.7 291.7 0
product-listing 280.9 280.9 0
product-recommendation 154.6 154.6 0
recommendation 190 190 0

@jelmedini jelmedini force-pushed the feature/SVCC-2942-result-actions branch from da8b0b3 to ec24b8b Compare June 26, 2023 20:55
@jelmedini jelmedini force-pushed the feature/SVCC-2942-result-actions branch from 658e40d to da3913f Compare June 26, 2023 21:33
@jelmedini jelmedini force-pushed the feature/SVCC-2942-result-actions branch from 927d112 to 95148db Compare June 27, 2023 00:00
lbergeron
lbergeron reviewed Jun 27, 2023
View reviewed changes
Copy link
Contributor

@lbergeron lbergeron left a comment

Choose a reason for hiding this comment

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

It looks quite nice!

louis-bompart
louis-bompart reviewed Jun 28, 2023
View reviewed changes
Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

Some :suspect: stuff to address

@jelmedini jelmedini force-pushed the feature/SVCC-2942-result-actions branch from 3846df6 to f5961b4 Compare June 28, 2023 19:37
@jelmedini jelmedini force-pushed the feature/SVCC-2942-result-actions branch from aae6992 to 4ee8afb Compare July 3, 2023 18:26
@jelmedini jelmedini force-pushed the feature/SVCC-2942-result-actions branch from 8ad4a69 to 8a5b814 Compare July 3, 2023 19:01
nathanlb
nathanlb approved these changes Jul 4, 2023
View reviewed changes
Copy link
Contributor

@nathanlb nathanlb left a comment

Choose a reason for hiding this comment

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

Still a couple comments but this is looking great 👍

@jelmedini jelmedini force-pushed the feature/SVCC-2942-result-actions branch from 7cd7df0 to f82f982 Compare July 5, 2023 11:41
@jelmedini jelmedini requested a review from louis-bompart July 5, 2023 14:25
@lvu285
Copy link
Contributor

lvu285 commented Jul 5, 2023

Nice work!
#1. Can you plz check to make sure that the action bar should not overlap with the item title when title doens't have badge + the title is very long (similar scenario like folding item _ but title is longer). Maybe it should be more consistent with the case when result has badge
#2. Can you plz check to make sure if only 1 action (1 icon) display, it still displays nicely _ include round corner
#3. From the comment of Nathan, I can see that we have the tooltips for these icons, just want to double confirm that we have it, as on the demo video, I don't see it.
Thanks

louis-bompart
louis-bompart requested changes Jul 5, 2023
View reviewed changes
Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

I'd prefer to see this !important gone.

@jelmedini jelmedini requested a review from louis-bompart July 5, 2023 20:58
@jelmedini
Copy link
Contributor Author

jelmedini commented Jul 5, 2023 •
edited
Loading

Nice work! #1. Can you plz check to make sure that the action bar should not overlap with the item title when title doens't have badge + the title is very long (similar scenario like folding item _ but title is longer). Maybe it should be more consistent with the case when result has badge #2. Can you plz check to make sure if only 1 action (1 icon) display, it still displays nicely _ include round corner #3. From the comment of Nathan, I can see that we have the tooltips for these icons, just want to double confirm that we have it, as on the demo video, I don't see it. Thanks

hey @lvu285 !

  • for point 1: the video wasn't updated but the result bar action is in between each result, EXCEPT the first result, as it is hidden if we do so, the layout component overflow is set to auto can't change that! but here it is how it looks like in screenshots. (I updated the demo video also)
  • point 2: yes I will include how it looks like when we have only one button
  • for tooltips it's actually implemented the same way as edit and toggle button as it is the same button component, they aren't quite intuitive I agree you need to be a little bit precise with the cursor.
    Screenshot 2023-07-05 at 4 46 08 PM

Screenshot 2023-07-05 at 4 44 21 PM

Screenshot 2023-07-05 at 4 43 52 PM

@lvu285
Copy link
Contributor

lvu285 commented Jul 6, 2023

awesome, only left 1 small comment, the rest is great. Thanks Jihan!
image

hey @lvu285 !

  • for point 1: the video wasn't updated but the result bar action is in between each result, EXCEPT the first result, as it is hidden if we do so, the layout component overflow is set to auto can't change that! but here it is how it looks like in screenshots. (I updated the demo video also)
  • point 2: yes I will include how it looks like when we have only one button
  • for tooltips it's actually implemented the same way as edit and toggle button as it is the same button component, they aren't quite intuitive I agree you need to be a little bit precise with the cursor.

@jelmedini jelmedini force-pushed the feature/SVCC-2942-result-actions branch from 2b9b082 to d1b3213 Compare July 6, 2023 15:27
@jelmedini
Copy link
Contributor Author

awesome, only left 1 small comment, the rest is great. Thanks Jihan! image

hey @lvu285 !

  • for point 1: the video wasn't updated but the result bar action is in between each result, EXCEPT the first result, as it is hidden if we do so, the layout component overflow is set to auto can't change that! but here it is how it looks like in screenshots. (I updated the demo video also)
  • point 2: yes I will include how it looks like when we have only one button
  • for tooltips it's actually implemented the same way as edit and toggle button as it is the same button component, they aren't quite intuitive I agree you need to be a little bit precise with the cursor.

Added an extra padding for first element when we have result action bar as discussed :)
Screenshot 2023-07-06 at 11 06 13 AM

louis-bompart
louis-bompart requested changes Jul 7, 2023
View reviewed changes
Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

Good chunks of logic could/should be handled through CSS #3019

louis-bompart and others added 2 commits July 7, 2023 16:00
@louis-bompart
embrace CSS (#3019)
cb38ca3 
* use CSS for hover

* handle first with CSS

* always relative
@jelmedini
Merge branch 'master' into feature/SVCC-2942-result-actions
0281ea4
@jelmedini jelmedini merged commit d2ed389 into master Jul 10, 2023
@jelmedini jelmedini deleted the feature/SVCC-2942-result-actions branch July 10, 2023 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lbergeron lbergeron lbergeron left review comments

@nathanlb nathanlb nathanlb approved these changes

@louis-bompart louis-bompart louis-bompart approved these changes

@olamothe olamothe Awaiting requested review from olamothe olamothe is a code owner automatically assigned from coveo/search

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jelmedini @nathanlb @lvu285 @lbergeron @louis-bompart