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

Bug - Badge no longer has spacing between chip text in chip #9483

Closed
kmcfaul opened this issue Aug 10, 2023 · 9 comments · Fixed by #9493
Closed

Bug - Badge no longer has spacing between chip text in chip #9483

kmcfaul opened this issue Aug 10, 2023 · 9 comments · Fixed by #9493
Assignees
Labels

Comments

@kmcfaul
Copy link
Contributor

kmcfaul commented Aug 10, 2023

Badges within chips should have spacing between it and the text.

Currently:
Content element > text element > badge element

Should be:
Content element > text element + badge element

Will address
RedHatInsights/frontend-components#1801

@kmcfaul kmcfaul added the bug label Aug 10, 2023
@MariaAga
Copy link
Contributor

This is caused because this react repo didnt add wrappers for content and actions for chips. should there be an issue to add them like here: patternfly/patternfly#5360 ?
@nicolethoen

@nicolethoen
Copy link
Contributor

Yes I believe that is how we will solve this issue 👍🏻

@MariaAga
Copy link
Contributor

Actually I now see thats its been done already, but the html examples is:

  <span class="pf-v5-c-chip__content">
    <span class="pf-v5-c-chip__text" id="chip_three">Chip</span>
    <span class="pf-v5-c-badge pf-m-read">00</span>
  </span>

while the the react one is:

<span class="pf-v5-c-chip__content">
  <span class="pf-v5-c-chip__text" id="pf-random-id-250">
    Chip
    <span class="pf-v5-c-badge pf-m-read">7</span>
  </span>
</span>

Should patternfly core add css to add spacing also between chip-text element, or somehow divide chips children between text children and react element children?

@nicolethoen
Copy link
Contributor

I believe the badge should be a sibling of the chip_text

@MariaAga
Copy link
Contributor

What would be the best way to achieve that? as now pf-react has all children under chip-text

<span ref={this.span} className={css(styles.chipText)} id={id}>
   {children}
</span>

@nicolethoen
Copy link
Contributor

hm.... sounds like it'd be a breaking change... @tlabaj @mcoker what do you think about this?

@mcoker
Copy link
Contributor

mcoker commented Aug 11, 2023

Hmm... looking at the core markup, it looks like it presumes the react <Chip> either 1) allows the user to do something like <Chip><ChipText>text</ChipText>[... a badge or whatever]</Chip>, or 2) the use of <Badge> is managed in the <Chip> via a prop like <Chip count=[...]>. The <Button countOptions> prop is similar https://www.patternfly.org/components/button#button-with-count

I'm not a huge fan of just applying a left margin to any badge in the chip since we don't always know where a chip would be used, though it would probably solve this issue well since we had that margin in v4. I would prefer we either support the badge in the chip via a prop like the button, and/or open it up to the user to manage the spacing since they may add it before or after the text, and put other stuff alongside it, too. In this case, a regular whitespace char works well enough as an interim fix.
Screenshot 2023-08-11 at 9 19 39 AM

@tlabaj
Copy link
Contributor

tlabaj commented Aug 14, 2023

@MariaAga please see the core issue here
patternfly/patternfly#5842 (comment)
@mcoker and I talked and decided to add the margin back in to css so those who have already upgraded will no longer be broken once the take the latest in. We also decided to add a prop to React for the badge rather that n passing it to the children. We will also write a code mode for this. That way whoever upgrades and used code modes will be using the correct API.
Will you have time to work on this in the next couple of days? Otherwise someone on THe PF team can make the change.

@MariaAga
Copy link
Contributor

@tlabaj Thanks for the answers! I have some pto so I cant do it this week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants