feat: #242 v5 badge component #273

DanishAli-Reapit · 2025-01-09T02:21:53Z

Description

Created a new v5 Badge component to reflect DS Figma
Added accessibility support to the badge

Badge.Story.mov

codacy-production · 2025-01-09T02:52:04Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for `a9c71e2`¹	✅ 100.00%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`a9c71e2`)	Report Missing	Report Missing	Report Missing
Head commit (`98594bb`)	4721	4206	89.09%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#273)	31	31	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

ksolanki7

praise: Congratulations on your 1st element implementation PR. There are some couple of suggestions I have left in the PR feedback.

ksolanki7 · 2025-01-09T05:37:01Z

src/components/badge/styles.ts

+import { css } from '@linaria/core'
+import { styled } from '@linaria/react'
+
+export const elBadgeLabel = css``


question: Is there a necessity of having a elBadgeLabel variable defined here, as I am unable to find it's usage.

Its use in the badge.tsx line number 36 {children && <ElBadgeLabel>{children}</ElBadgeLabel>}

issue (non-blocking): ElBadgeLabel is already instantiated for the styled div here. To utilize it effectively, you only need to define this variable for instantiation, as the className is specifically used for targeted styling. For example:

export ElBadge = styled.div` .${elBadgeLabel} { .... } ... `

I will remove the const declare in line 4 as it is not used in the file.

ksolanki7 · 2025-01-09T06:29:35Z

src/components/badge/badge.tsx

+
+interface BadgeProps extends HTMLAttributes<HTMLDivElement> {
+  children?: ReactNode
+  reversed?: boolean


suggestion: I would prefer to use boolean props with an auxiliary verb i.e. isReversed. reversed indicates a condition (the background color is reversed or not), making it a state representing a true/false value. It could be more intuitive if named something like isReversed or reverse to clearly reflect that it's a boolean value denoting the current state of the background.

Thanks good suggestion I will update my PR.

kurtdoherty

praise: As @ksolanki7 said, congrats on your first contribution to Elements 👏 Lot of good work here; particularly nice to see some consideration going to the storybook control values 👍

kurtdoherty · 2025-01-14T03:37:58Z

src/components/badge/badge.tsx

+  iconLeft?: ReactNode
+  iconRight?: ReactNode
+  'aria-label'?: string
+  className?: string


quibble: aria-label and className shouldn't need to be included explicitly; they'll be part of the HTMLAttributes<HTMLDivElement> interface that we're extending here.

Thanks @kurtdoherty for the feedback. I will remove them from my PR.

kurtdoherty · 2025-01-14T03:40:47Z

src/components/badge/badge.tsx

+  return (
+    <ElBadge
+      data-variant={variant}
+      data-isReversed={isReversed}


issue: data-* attributes should use snake/hyphen case. That is, this should be data-is-reversed. Doing this ensures JavaScript access of the data attribute has the camel casing we'd expect (isReversed). You can read more about this in the MDN docs on using data attributes

Thanks @kurtdoherty for the feedback. I will update my PR accordingly.

kurtdoherty · 2025-01-14T03:43:02Z

src/components/badge/styles.ts

+import { css } from '@linaria/core'
+import { styled } from '@linaria/react'
+
+export const elIcon = css``


question: I don't see this used in the actual Badge component as a wrapper around the iconLeft or iconRight. So is this class actually doing what the CSS below suggests it is doing?

Thanks @kurtdoherty for the feedback. Yes, this class was not required. I have updated my PR. Please review.

kurtdoherty · 2025-01-14T05:44:00Z

src/components/badge/styles.ts

+  color: var(--text-info);
+  background: var(--fill-info-lightest);
+
+  .el-icon {


issue: I think we've misunderstood each other. Previously, you had a local elIcon CSS class defined that you were using here. That was fine, it just wasn't being used explicitly in our Badge.

Now, you've hardcoded what I assume is the Icon component's class within our Badge component's CSS. This creates an implicit (hidden), tight coupling between our Badge component and our Icon component. We don't want to do this, because it means the behaviour owned by our Badge (changing the icon colour) will only work if our Icon component is used to handle the display of the icon and that Icon component does not change the name of it's primary class.

Until we implement a new Icon component, or update how the existing one is implemented so that it respects the colour of its ancestor element, we want to do two things:

We want to implement our Badge in a way that is agnostic of what component is used to render the icon. What we want is to wrap whatever icon element is provided with our own element that applies the styles we want (like the color); and,

Add a well-commented, explicit dependency on the ElIcon styled component so that:
a. We force ElIcon to have color: inherit; and,
b. It's easy for us to delete that "patch" when we refactor/replace Icon.

I believe this is what you'll see in some of the other components we've recently worked on that also need to influence the colour of icons provided via ReactNode props (IIRC, MobileNavItem may be taking this approach)

Thanks @kurtdoherty for the review. I was not thinking that way. I have now updated my PR and forced ElIcon to have a color: inherit

kurtdoherty · 2025-01-16T03:59:17Z

src/components/badge/styles.ts

@@ -24,135 +25,75 @@ export const ElBadge = styled.div`
  &[data-is-reversed='true'] {
    color: var(--text-white);
    background: var(--fill-info-dark);
-


question: since the icon's are using different CSS variables than the badge text colour, don't we still need to wrap the icon in an element controlled by our Badge component and have styles for that element similar to what you had here?

I know the Icon has different CSS variables but the value is the same and by using the icon color inherit it will use the same color as the label text color. If we want to use the icon CSS variables then I can't declare the icon color as inherit I have to use the icon color like var(--icon-white)

Hmm, when we're using CSS variables, I think we should be focused on using the correct, semantic CSS variable in the correct context. Allowing icons to be coloured based on a text-related CSS variable means we're misusing that CSS variable, even if it happens to have the same value as it's icon-related counterpart. Ultimately, I don't actually understand why Design have separate text and icon colour design tokens, but it's what we currently have to work with.

In a previous comment about the relationship between Badge and Icon, I communicated the following:

Until we implement a new Icon component, or update how the existing one is implemented so that it respects the colour of its ancestor element, we want to do two things:

We want to implement our Badge in a way that is agnostic of what component is used to render the icon. What we want is to wrap whatever icon element is provided with our own element that applies the styles we want (like the color); and,

Add a well-commented, explicit dependency on the ElIcon styled component so that:
a. We force ElIcon to have color: inherit; and,
b. It's easy for us to delete that "patch" when we refactor/replace Icon.

I'll try to expand on what I meant by (1) and (2)...

By (1), I mean our structure probably needs to be:

<ElBadge> // <--- sets "color: var(--text-white);" <ElBadgeIcon>...</ElBadgeIcon> // <---- sets "color: var(--icon-white);" <ElBadgeLabel>Label</ElBadgeLabel> </ElBadge>

By (2), I mean, our CSS for ElBadgeIcon (or ElBadge) monkey-patches/overrides descendant ElIcon elements to ensure they inherit their ancestor's colour, which will be what ElBadgeIcon has set.

More than happy for alternative suggestions, but this is the only way I can see at the moment for us to have a good change of correctly colouring whatever icon is supplied to the Badge component, while also ensuring use of the current Elements Icon component behaves how we need it to when inside a Badge.

Thanks @kurtdoherty for the feedback. I have updated my with your suggestions also applied the same on the split-button component.

…so updated storie for color variant

kurtdoherty

Thanks for your patience with this one @DanishAli-Reapit 🙏 I think we can merge it in now.

feat: #242 created v4 badge component

e0cf5cd

DanishAli-Reapit linked an issue Jan 9, 2025 that may be closed by this pull request

v5 component refactor: Badge #242

Closed

14 tasks

DanishAli-Reapit mentioned this pull request Jan 9, 2025

v5 component refactor: Badge #242

Closed

14 tasks

feat: #242 fixed es lint issue

2a56414

Danish Ali added 5 commits January 9, 2025 14:17

[Ignore] pushing an extra line to push index.ts file

9e66434

Removing an extra line to push index.ts file

8d112ee

[Ignore] pushing an extra line to push index.ts file

376ab96

Removing an extra line to push index.ts file

a63f8ef

[Ignore] push index.ts file

4e15ca8

DanishAli-Reapit requested review from kurtdoherty and ksolanki7 January 9, 2025 04:24

ksolanki7 reviewed Jan 9, 2025

View reviewed changes

ksolanki7 self-requested a review January 9, 2025 06:33

Danish Ali added 2 commits January 10, 2025 12:39

feat: #242 updated v5 badge component with the PR review

f88c4e9

feat: #242 removed extra export from the styles

97253c1

kurtdoherty reviewed Jan 14, 2025

View reviewed changes

Danish Ali and others added 3 commits January 14, 2025 15:30

feat: #242 updated PR with the PR feedback

ef03a75

feat: #242 updated snapshot test

b0a45c1

Merge branch 'main' into feat-#242-v5-Badge-component

b708fee

kurtdoherty reviewed Jan 14, 2025

View reviewed changes

feat: #242 forced ElIcon to have a color inherit

85c52a4

kurtdoherty reviewed Jan 16, 2025

View reviewed changes

DanishAli-Reapit and others added 3 commits January 17, 2025 14:22

Merge branch 'main' into feat-#242-v5-Badge-component

09e2d76

feat: #242 wrapped icon component with span to pass the icon color al…

21ef2c4

…so updated storie for color variant

feat: #242 updated test

e7f7a95

kurtdoherty approved these changes Jan 23, 2025

View reviewed changes

Merge branch 'main' into feat-#242-v5-Badge-component

98594bb

DanishAli-Reapit merged commit 79a2199 into main Jan 23, 2025
6 checks passed

DanishAli-Reapit deleted the feat-#242-v5-Badge-component branch January 23, 2025 05:21

DanishAli-Reapit self-assigned this Jan 29, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: #242 v5 badge component #273

feat: #242 v5 badge component #273

DanishAli-Reapit commented Jan 9, 2025 •

edited

Loading

codacy-production bot commented Jan 9, 2025 •

edited

Loading

ksolanki7 left a comment

ksolanki7 Jan 9, 2025 •

edited

Loading

DanishAli-Reapit Jan 10, 2025

ksolanki7 Jan 13, 2025

DanishAli-Reapit Jan 13, 2025

ksolanki7 Jan 9, 2025

DanishAli-Reapit Jan 10, 2025

kurtdoherty left a comment

kurtdoherty Jan 14, 2025

DanishAli-Reapit Jan 14, 2025

kurtdoherty Jan 14, 2025

DanishAli-Reapit Jan 14, 2025

kurtdoherty Jan 14, 2025

DanishAli-Reapit Jan 14, 2025

kurtdoherty Jan 14, 2025 •

edited

Loading

DanishAli-Reapit Jan 14, 2025

kurtdoherty Jan 16, 2025

DanishAli-Reapit Jan 16, 2025

kurtdoherty Jan 16, 2025 •

edited

Loading

DanishAli-Reapit Jan 20, 2025

kurtdoherty left a comment

feat: #242 v5 badge component #273

feat: #242 v5 badge component #273

Conversation

DanishAli-Reapit commented Jan 9, 2025 • edited Loading

Description

codacy-production bot commented Jan 9, 2025 • edited Loading

Coverage summary from Codacy

Footnotes

ksolanki7 left a comment

Choose a reason for hiding this comment

ksolanki7 Jan 9, 2025 • edited Loading

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

kurtdoherty 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

kurtdoherty Jan 14, 2025 • edited Loading

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

kurtdoherty Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurtdoherty left a comment

Choose a reason for hiding this comment

DanishAli-Reapit commented Jan 9, 2025 •

edited

Loading

codacy-production bot commented Jan 9, 2025 •

edited

Loading

ksolanki7 Jan 9, 2025 •

edited

Loading

kurtdoherty Jan 14, 2025 •

edited

Loading

kurtdoherty Jan 16, 2025 •

edited

Loading