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

LF-4079 Create reusable group pill component #3115

Merged
merged 12 commits into from
Feb 12, 2024

Conversation

kathyavini
Copy link
Collaborator

@kathyavini kathyavini commented Feb 6, 2024

TODO:

  • Confirm the status of the new dark blue with Loïc
    • All the new colours are part of the new design system

Description

  • This creates the + {{count}} others Hover Pill for use on the Group column of the Animal table
  • The hover pill text adapts to count (@antsgar I am leaving gender as a problem to solve the first time we encounter an inconsistency!)
  • Storybook There is a property "position" (property of the Storybook story, not of the component) which allows you to place the hover pill in different corners of the Storybook iFrame to watch the Popper/MUI tooltip adapt to the viewpoint edges. Which isn't even our code anyway so it was a bit pointless to test, but I really wanted to see it working 😂

Small note: on mobile the distance from anchor to tooltip is further than on Desktop. This is true for the other MUI Tooltip in app (the <Infoi /> component) as well and I am not sure the reason.

Jira link: https://lite-farm.atlassian.net/browse/LF-4079

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Passes test case
  • UI components visually reviewed on desktop view
  • UI components visually reviewed on mobile view
  • Other (please explain)

Only viewed in Storybook UI as not yet present in App.

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • The precommit and linting ran successfully
  • I have added or updated language tags for text that's part of the UI
  • I have added "MISSING" for all new language tags to languages I don't speak
  • I have added the GNU General Public License to all new files

@kathyavini kathyavini self-assigned this Feb 6, 2024
@kathyavini kathyavini requested review from a team as code owners February 6, 2024 21:08
@kathyavini kathyavini requested review from antsgar and SayakaOno and removed request for a team February 6, 2024 21:08
Copy link
Collaborator

@SayakaOno SayakaOno left a comment

Choose a reason for hiding this comment

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

Super quick, and looks great!

I wonder how flexible this component should be.
Screenshot 2024-02-06 at 1 26 16 PM
If there is a pill at the bottom of the window and the number is 5 for example, the tooltip will be cut off.

I'm not sure what Loïc's solution will be, but I think it would be nice if the placement changes depending on where in the window the pill is. (it might be easy to implement it with a Mui component https://mui.com/material-ui/react-tooltip/)

@kathyavini
Copy link
Collaborator Author

@SayakaOno Oh, that is the kind of thing I would never have considered! 😂

Thank you for the MUI component link. I'll give it a try and also see if I can fix the story to be better about positioning.

Add position property and new decorator to allow casting component to different parts of the Storybook frame
@kathyavini kathyavini marked this pull request as draft February 7, 2024 02:32
@kathyavini kathyavini marked this pull request as ready for review February 7, 2024 06:27
@antsgar
Copy link
Collaborator

antsgar commented Feb 7, 2024

@loicsans some strings to translate here too

@kathyavini kathyavini added the enhancement New feature or request label Feb 7, 2024
Copy link
Collaborator

@SayakaOno SayakaOno left a comment

Choose a reason for hiding this comment

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

Looks great!
The options for the position in the story are very nice! Thank you ❤️
I'll approve once the changes are finalized.

Comment on lines +58 to +59
enterTouchDelay={0}
leaveTouchDelay={900000}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason we're not using the defaults here?

Copy link
Collaborator Author

@kathyavini kathyavini Feb 8, 2024

Choose a reason for hiding this comment

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

These values come from the other MUI tooltip we have in app (the Info icon tooltip). Personally my reaction to the MUI default (that you have to long press and not tap) was to think the hover pill was broken on mobile! I think probably that's how the Infoi got its settings too. But is the long press interaction more standard?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I have no idea which one would be more common tbh! But that makes sense to me

{
name: 'offset',
options: {
offset: [0, -8],
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's something super small that's bothering me 😅 which I think is actually buggy behavior from MUI, but hopefully we can fix it? When you hover over the pill and the tooltip opens, if you then hover back to the pill there's a space within it where hovering doesn't work and the styling turns back to the "unhovered" style. I recorded a video which hopefully shows it better.

Grabacion.de.pantalla.2024-02-08.a.la.s.10.02.49.mov

I couldn't figure out why this was happening, but it turns out that the tooltip has a bottom margin and that bit of it is overlapped with the pill.

Captura de pantalla 2024-02-08 a la(s) 10 05 28

The solution seems to be to apply a margin: 0 style to the tooltip to override MUI's margin, and then adjust the offset to be a positive number so that there's still some space between the tooltip and the pill. The only problem is that the MUI selector is more specific than the .hoverDetails one, so for it to work the custom selector needs to be more specific. One way that I found to do this without using !important is to apply the hoverDetails styling to MUI's styling for specific tooltip placements, like this:

<Tooltip
      classes={{
        tooltip: styles.hoverDetails,
        tooltipPlacementTop: styles.hoverDetails,
      }}
      ...
>

Copy link
Collaborator Author

@kathyavini kathyavini Feb 8, 2024

Choose a reason for hiding this comment

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

Ah, so that's why the offset had to be negative in the first place! These default component styles always get me!! 🤣

I couldn't get it to work just like you described though (with override classes but without !important); could you check again? 😅 And once the overall offset is changed to positive I think we'll need to override classes for more placements (I would think at least tooltipPlacementBottom)? If so I think it might be okay (and at least more succinct) to go the !important route...

(I'll commit the !important version which does fix the bug, but if you could check the code again...!)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah no, you're right, it doesn't work, I had probably done a different change and thought it worked when it didn't. At some point I just repeated the same selector to increase the specificity, like .hoverDetails.hoverDetails.hoverDetails but I think anyone reading that would wonder what's going on there probably 😅 so I'm alright with keeping the important. For the record this is one of the things I strongly dislike about MUI, which is how hard it is to override their styling

@antsgar
Copy link
Collaborator

antsgar commented Feb 8, 2024

Looks great and the story works great too! There's a conflict from your other PR which I just merged, and I added a comment for a styling detail that's bugging me but other than that it's perfect 🙂

@antsgar antsgar self-requested a review February 8, 2024 19:42
@kathyavini kathyavini requested a review from SayakaOno February 9, 2024 17:08
@SayakaOno SayakaOno added this pull request to the merge queue Feb 12, 2024
Merged via the queue into integration with commit 8b1e594 Feb 12, 2024
5 checks passed
kathyavini added a commit that referenced this pull request Feb 20, 2024
Also added the _many for the COUNT_OTHERS from PR #3115 -- although the component doesn't need it -- as the i18n script was automatically generating it. Translated total (it's the same in all languages??) as it was present in previous table headings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants