LF-4374 Badge component #3486

gursimran-singh · 2024-10-02T19:47:52Z

Description

A reusable badge component that shows the content when onHover - desktop, onClick - mobile, desktop. Usage:
<Badge title='Beta' content='This is the content which will be displayed onHover - desktop, onClick - mobile, desktop' />

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

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)

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

antsgar

@gursimran-singh thank you so much for working on this, we really appreciate it. It looks great! 🙌

I left a couple of comments throughout, and additionally:

In the designs, when the popover is open, the badge's bottom border has a smaller border radius (half of the normal radius) to sort of align with the popover.
The ticket description includes adding the Beta badge to different Animals components, were you planning on tackling that in a separate PR? If you can't get to it it's no problem, we can split it up to a separate ticket.

packages/webapp/src/components/Badge/index.tsx

packages/webapp/src/components/Badge/styles.module.scss

packages/webapp/src/components/Badge/index.tsx

packages/webapp/src/components/Badge/styles.module.scss

gursimran-singh · 2024-10-07T19:55:10Z

@antsgar

I will do the mentioned changes. And regarding adding Beta badge to different Animals components, to what i remember when having discussion with team is that these can be added later(i might have got it wrong). I will add the Badge component to required places and will get in touch if i have any questions/issues doing that. Thanks.

…ta-badge-and-add-to-animals-menu-item

antsgar

Looks great! Not a blocker for merging (we can fix later), but on second thought I think on desktop the tooltip content should appear/disappear on click instead of on hover. It seems strange to me that the badge changes appearance on click and when clicking outside of it, but the tooltip interaction happens on hover. I'll double check with design tomorrow though.

On the other hand, I think there's a different design for the tooltip on mobile, which is full screen. I'll double check this tomorrow as well and let you know!

antsgar · 2024-10-21T21:21:00Z

packages/webapp/src/components/Badge/index.tsx

-    >
-      {content}
-    </div>
+      dangerouslySetInnerHTML={{ __html: content }}


Could you explain the reason behind this change? Instead of setting the link as HTML within the content string, I think we should keep this as is and set the content up as a React Node including the tag

My intentions when developing the component was to make it reusable with different title and content in any other section. So, do not want to hard code anything in the component itself. I will change it to use <Trans> component from 'react-i18next' package(which i found later), is safer way to handle these situations and set the link when passing the content to the component.

Also, it would be nice if you can tell the link address, then i will update that as well.

packages/webapp/public/locales/en/translation.json

gursimran-singh · 2024-10-22T07:47:54Z

I had discussion with team and showed them the functionality and they were ok with it. Only change was that the badge in the menu items should no show the content popup in any case.

I think there's a different design for the tooltip on mobile, which is full screen
We had a discussion on this as well, showing on full screen is a part of spotlight feature and in mobile content should only be shown on click as we do not have hover there.

Do let me know if there are changes and i will do it accordingly.

Duncan-Brain

Hey @gursimran-singh ! Thanks for working on this, so sorry it took a while for us to get a review.

I think overall it is looking great I just found a few small things:

The onClick behaviour does not quite work as expected on mobile (click does not work as click out) and desktop (click changes styles but does not keep tooltip open) for please see video

Screen.Recording.2024-10-25.at.12.09.29.PM.mov

Screen.Recording.2024-10-25.at.12.10.09.PM.mov

Couple inline comments
Would you be willing to make storybook stories for this new component?

Duncan-Brain · 2024-10-25T16:17:37Z

packages/webapp/src/components/Badge/index.tsx

+      setOpen(true);
+    }
+  };
+  const handleTooltipClose = () => {


Could handleTooltipOpen + handleTooltipClose be combined by letting the state setter to use previous/pending value. React reference if useful

setOpen(prevState => !prevState)

That can be done if we want the button to behave like toggle(one click open tooltip and another closes it). Honestly speaking, i am little unclear about the behaviour of this component and that is why i came to the meeting to show you guys what i build. Got some feedback and changed accordingly. Even Antonella is not sure about the hover/click functionality for both mobile and desktop and told me that she would let me know after discussing with the team.

So, do let me know what should be the behaviour of this component for onHover and click, for both mobile and desktop and i will make changes accordingly

Honestly its mostly fine the way you have it as an only onHover on desktop and onClick on mobile.

But specifically the style of the button on desktop should not change onClick on desktop though as shown in the first video... unless the full click behaviour opens the tooltip. So either we need to disable click styles on desktop, or add the full onClick open behaviour. I would personally approve either of those two options.

I will tag Loic on the Jira ticket to hopefully get you clarification on this though so you feel confident. Sorry about that lack of clarity -- please always feel free to tag Loic with your UI/UX questions on the Jira ticket too!

packages/webapp/src/components/Badge/index.tsx

SayakaOno · 2024-10-25T17:41:10Z

packages/webapp/src/components/Badge/styles.module.scss

+:global(.hasBadge) .badge {
+  height: 12px;
+  font-size: 11px;
+  padding: 6px;
+  position: absolute;
+  left: 50px;
+  top: 8px;
+  border-radius: 16px;
+  &:focus,
+  &:hover {
+    color: var(--Colors-Accent-Accent-yellow-600);
+    background: #fffbf2;
+    border-radius: 16px !important;
+  }
+
+  @include xs-breakpoint {
+    height: 16px;
+    position: relative;
+    font-size: 14px;
+    top: initial;
+    left: initial;
+    padding: 6px;
+    margin-right: 6px;
+  }
+}


Would it be a bad idea to pass a className with these styles to the Badge component instead?
I've never seen a raw CSS className used here before, so it would be nice if we could keep things consistent!

It is one of the way by which we can access the parent class in the child component. But, i will see these styles can be attained by passing class to the component and also avoiding the inline css.

SayakaOno · 2024-10-25T17:52:58Z

packages/webapp/src/components/Badge/index.tsx

+  style?: React.CSSProperties;
+}
+
+const Badge: React.FC<BadgeProps> = ({ content = '', title, showIcon = true, style }) => {


We're aiming to avoid inline CSS, so could you please change the style prop to className instead? 🙏

Its good we are avoiding the inline css, i have seen a lot here. And sure, i will update accordingly.

LF-4374 Badge component

aa9be7b

gursimran-singh requested review from a team as code owners October 2, 2024 19:47

gursimran-singh requested review from antsgar and kathyavini and removed request for a team October 2, 2024 19:47

antsgar reviewed Oct 4, 2024

View reviewed changes

gursimran-singh added 3 commits October 15, 2024 23:41

LF-4374 Added badge to animal menu

a4512bc

Merge branch 'integration' into LF-4374-Beta-Badge-Create-reusable-be…

7ec52dd

…ta-badge-and-add-to-animals-menu-item

LF-4374 Added language string to different translation files

314d21b

gursimran-singh requested a review from antsgar October 16, 2024 05:58

LF-4374 Removed tooltip on hover from menu items

3940db8

antsgar reviewed Oct 21, 2024

View reviewed changes

LF-4374 Used <Trans> to handle link in translation

5308185

Duncan-Brain requested review from Duncan-Brain and removed request for kathyavini October 25, 2024 16:02

Duncan-Brain reviewed Oct 25, 2024

View reviewed changes

SayakaOno reviewed Oct 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LF-4374 Badge component #3486

LF-4374 Badge component #3486

gursimran-singh commented Oct 2, 2024

antsgar left a comment

gursimran-singh commented Oct 7, 2024

antsgar left a comment

antsgar Oct 21, 2024

gursimran-singh Oct 22, 2024 •

edited

Loading

gursimran-singh commented Oct 22, 2024

Duncan-Brain left a comment

Duncan-Brain Oct 25, 2024

gursimran-singh Oct 28, 2024

Duncan-Brain Oct 29, 2024

SayakaOno Oct 25, 2024

gursimran-singh Oct 28, 2024

SayakaOno Oct 25, 2024

gursimran-singh Oct 28, 2024

LF-4374 Badge component #3486

Are you sure you want to change the base?

LF-4374 Badge component #3486

Conversation

gursimran-singh commented Oct 2, 2024

antsgar left a comment

Choose a reason for hiding this comment

gursimran-singh commented Oct 7, 2024

antsgar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gursimran-singh Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

gursimran-singh commented Oct 22, 2024

Duncan-Brain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gursimran-singh Oct 22, 2024 •

edited

Loading