feat(components): update Chip.Prefix to accept more than Icon and Avatar #2020

demoraesgui · 2024-09-10T19:49:52Z

Motivations

It would be nice to be able to add like a logo or a custom image to the prefix

Changes

Chip.Prefix now accepts anything, but if you provide either an Avatar or an Icon you get the fallback behavior

Changes can be
tested via Pre-release

In Atlantis we use Github's built in pull request reviews.

cloudflare-workers-and-pages · 2024-09-10T19:52:40Z

Deploying atlantis with Cloudflare Pages

Latest commit:	`0ae367e`
Status:	✅ Deploy successful!
Preview URL:	https://d1695116.atlantis.pages.dev
Branch Preview URL:	https://feat-update-chip-to-allow-mo.atlantis.pages.dev

View logs

ZakaryH · 2024-09-17T16:17:59Z

packages/components/src/Chip/components/ChipPrefix/Chip.Prefix.tsx

    children,
    d => d.type === Avatar || d.type === Icon,
  );

-  return (
-    <span className={classNames(styles.prefix, !singleChild && styles.empty)}>


I think we still need the behavior from the styles.empty

what the empty style is doing is setting display: none on the span in the event that we didn't receive a valid child

without that, if you do <Chip.Prefix/> we are rendering an inline-block span that doesn't really need to be there at all

if I'm understanding this correctly, the reason it was done like this previously was that we might have children, but we had to check what type they were before we could know if they were valid.

typically if you don't want something showing up, you should just not render it at all one level rather than hiding it or returning null inside the component itself.

I think now though, we can know that up in Chip if prefix.props.children is nothing, then we have nothing to render as a Prefix, and we shouldn't render it at all which skips us having to add the display: none style altogether.

const prefixHasContent = prefix?.props.children ... {prefixHasContent && prefix}

the variable assignment might be overkill. prefix?.props.children is pretty easy to understand, and might not need a descriptive name.

that should also remove the need for any of the avatarOrIcon logic too. we don't care what you gave us, we'll just render the children. keeps the Prefix dumb, and Chip is now responsible for making sure we're using Prefix when it makes sense.

Thanks for the feedback @ZakaryH I'm going to update the PR in a couple hours.

oh, actually around my comment about not needing avatarOrIcon - I might have misunderstood the purpose

is that so that we only apply the styles.prefix to that type of children? everything else, the consumer is responsible for the layout styles and encapsulating markup?

@ZakaryH avatarOrIcon was also added because there is some styling being added to ensure the Avatar or Icon gets border-radius: var(--radius-circle);

This style shouldn't be enforced to a custom prefix. How you want me to handle this?

interesting. ok yeah then we might still need that after all.

though I don't really love the idea of different compositions behaving differently.

if I do

<Chip> <Chip.Prefix><Icon name="whatever"/></Chip.Prefix> </Chip>

this renders markup that I don't have control over

whereas

<Chip> <Chip.Prefix><StatusLabel variation="whatever"/></Chip.Prefix> </Chip>

this will render exactly what I gave it

in a weird way, we're making Icon/Avatar compositions more restricted. though we do have to consider all the existing uses that do expect us to be taking care of the layout. that's tricky.

we want a consistent API, with equal freedom regardless of what it is given, but also we don't want to break all current uses 😅

yeah, that's tricky. But regardless of how we deal with this styling, the prefixHasContent guard feels like something we would still want.

agreed. the conditional rendering avoids the empty style, and I like it better overall.

I'm thinking about how to handle this other problem and if there's a way for us to get everything we want...

ZakaryH · 2024-09-17T16:23:49Z

couple other thoughts:

Chips/Chip - it has similar restrictions on the Prefix. I wonder if we should do something similar there.

Suffix - same idea. should we apply this freedom there too, and on both implementations?

I know that's quite a bit more scope, and if this Chip.Prefix is all you need - then that's fair. I don't want to sign you up for a bunch of other work. however I do want to make sure we're keeping track of inconsistencies, and things that we should follow up on if we think this is the direction we want to go with Prefix/Suffix on Chip

demoraesgui · 2024-09-17T16:29:05Z

couple other thoughts:

Chips/Chip - it has similar restrictions on the Prefix. I wonder if we should do something similar there.

Suffix - same idea. should we apply this freedom there too, and on both implementations?

I know that's quite a bit more scope, and if this Chip.Prefix is all you need - then that's fair. I don't want to sign you up for a bunch of other work. however I do want to make sure we're keeping track of inconsistencies, and things that we should follow up on if we think this is the direction we want to go with Prefix/Suffix on Chip

That's fair, I brought up some of this on the last office hours. I think I'm going to join office hours again if I get blocked.
We only need the prefix change, but I'm going to take a look at the suffix as well if the changes aren't that bad we can add on this PR. Not sure about Chips though, if the changes aren't that crazy I'm going to add them as well.

demoraesgui · 2024-09-17T16:58:39Z

@ZakaryH looking at the amount of conditional logic in Chip.Suffix I feel like including its changes on this PR will increase a lot the complexity. Can this be something we track in Atlantis backlog instead?

There are keyboard/mouse events, allowed children (subset of icons) and accessibility as well. 😅

ZakaryH · 2024-09-17T17:00:57Z

@ZakaryH looking at the amount of conditional logic in Chip.Suffix I feel like including its changes on this PR will increase a lot the complexity. Can this be something we track in Atlantis backlog instead?

There are keyboard/mouse events, allowed children (subset of icons) and accessibility as well. 😅

yeah that's fair. I kind of thought that would be the case. we can make a note of that and track the work for later.

thanks for looking/trying!

…tar as children

… content presence

ZakaryH · 2024-09-17T18:01:29Z

@demoraesgui alright, after thinking about this more and talking with a teammate I think it's fine to have the conditional extra span and styles.prefix

existing usages will continue to work and be simple. new usages will have the freedom to build whatever layout they need.

then, if for some reason you want a custom layout with an Avatar or Icon, you can simply wrap it in a fragment or div or span or anything really, and that will avoid the Icon and Avatar type check

the only thing I'd suggest is that we document this behavior and say something like

When Chip.Prefix is provided an Icon or Avatar as the immediate child or children, some extra markup and styles will automatically be added. If these styles and markup are not desired, you may wrap the Avatar(s) or Icon(s) in any other element and provide your own layout.

ZakaryH · 2024-09-17T21:47:32Z

packages/components/src/Chip/Chip.tsx

@@ -33,6 +33,8 @@ export const Chip = ({
  const prefix = useChildComponent(children, d => d.type === Chip.Prefix);
  const suffix = useChildComponent(children, d => d.type === Chip.Suffix);

+  const prefixHasContent = !!prefix?.props?.children;


@demoraesgui

whoops, I was looking at the wrong span this whole time. the one you can see in my first screenshot is the labelRef span which is always there.

the prefix span will not be rendered anyway because if you pass nothing as a child to Chip.Prefix then children is undefined and we do

if (!avatarOrIcon) return children as React.ReactElement;

so what you had originally did work for not rendering the span. my bad! we can get rid of this prefixHasContent and conditional rendering.

though that does make me question the as React.ReactElement. we're overriding TypeScript and I'm not sure we should be. without this casting, it is correctly going to complain that Chip.Prefix could be undefined which is not a valid JSX Element.

instead of casting it as React.ReactElement which might not be true, inside Chip.Prefix let's do

if (!avatarOrIcon) return <>{children}</>;

that ensures we're always returning valid JSX, and if it's empty then it won't render anything

also, this will make sure that passing multiple elements is valid. I don't exactly know what you'd be doing, but this isn't valid right now because it has no surrounding element

Oh I see, I had that before, and the as React.ReactElement was something we added in an office hours. I'll update the PR in a bit. Thanks!

@demoraesgui ah sorry I wasn't there for office hours

do you remember why we didn't want to use a fragment?

if there's a good reason and we've already decided to use the casting I would be open to doing it this way, I just don't like casting things in general - and I know there's at least 1 React error that this casting will suppress

TBH I prefer not casting as well. I don't recall if there was a particular reason. It was more like, typescript is breaking and this could be an option to fix it.

ZakaryH · 2024-09-18T18:32:54Z

tried it out with a bunch of different prefix content

no TS errors even with multiple elements passed in as immediate children, nor when no content passed - also for the latter the extra span for the prefix is not being rendered

ZakaryH

LGTM thanks for making this PR and dealing with all the back and forth!

demoraesgui · 2024-09-18T18:34:15Z

No worries, thanks for great feedback and QA :)

demoraesgui marked this pull request as ready for review September 11, 2024 20:26

demoraesgui requested a review from a team as a code owner September 11, 2024 20:26

jobbiebot bot added review requested Team Atlantis labels Sep 11, 2024

demoraesgui self-assigned this Sep 11, 2024

taylorvnoj self-requested a review September 17, 2024 14:27

ZakaryH reviewed Sep 17, 2024

View reviewed changes

demoraesgui added 3 commits September 17, 2024 14:22

feat(components): Update Chip.Prefix to accept more than Icon and Ava…

aea28aa

…tar as children

test: update tests for Chip.Prefix to cover new behavior

2228d01

feat(components): Update Chip.Prefix to conditionally render based on…

9c0b589

… content presence

demoraesgui force-pushed the feat/update-chip-to-allow-more-than-icon-and-avatar branch from e86679b to 9c0b589 Compare September 17, 2024 17:22

Add Chip.Prefix behavior to the docs

17a2f80

demoraesgui requested a review from ZakaryH September 17, 2024 20:07

ZakaryH reviewed Sep 17, 2024

View reviewed changes

refactor: use fragment isntead of casting and remove unnecessary guard

0ae367e

demoraesgui requested a review from ZakaryH September 18, 2024 17:56

ZakaryH approved these changes Sep 18, 2024

View reviewed changes

jobbiebot bot added approved and removed review requested labels Sep 18, 2024

ZakaryH merged commit 1a6c72c into master Sep 18, 2024
11 checks passed

ZakaryH deleted the feat/update-chip-to-allow-more-than-icon-and-avatar branch September 18, 2024 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(components): update Chip.Prefix to accept more than Icon and Avatar #2020

feat(components): update Chip.Prefix to accept more than Icon and Avatar #2020

demoraesgui commented Sep 10, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Sep 10, 2024 •

edited

Loading

ZakaryH Sep 17, 2024 •

edited

Loading

demoraesgui Sep 17, 2024

ZakaryH Sep 17, 2024 •

edited

Loading

demoraesgui Sep 17, 2024

demoraesgui Sep 17, 2024 •

edited

Loading

ZakaryH Sep 17, 2024

demoraesgui Sep 17, 2024 •

edited

Loading

ZakaryH Sep 17, 2024

ZakaryH commented Sep 17, 2024

demoraesgui commented Sep 17, 2024 •

edited

Loading

demoraesgui commented Sep 17, 2024 •

edited

Loading

ZakaryH commented Sep 17, 2024

ZakaryH commented Sep 17, 2024

ZakaryH Sep 17, 2024 •

edited

Loading

demoraesgui Sep 18, 2024

ZakaryH Sep 18, 2024

demoraesgui Sep 18, 2024

ZakaryH commented Sep 18, 2024

ZakaryH left a comment

demoraesgui commented Sep 18, 2024

feat(components): update Chip.Prefix to accept more than Icon and Avatar #2020

feat(components): update Chip.Prefix to accept more than Icon and Avatar #2020

Conversation

demoraesgui commented Sep 10, 2024 • edited Loading

Motivations

Changes

cloudflare-workers-and-pages bot commented Sep 10, 2024 • edited Loading

Deploying atlantis with Cloudflare Pages

ZakaryH Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

demoraesgui Sep 17, 2024

Choose a reason for hiding this comment

ZakaryH Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

demoraesgui Sep 17, 2024

Choose a reason for hiding this comment

demoraesgui Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

ZakaryH Sep 17, 2024

Choose a reason for hiding this comment

demoraesgui Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

ZakaryH Sep 17, 2024

Choose a reason for hiding this comment

ZakaryH commented Sep 17, 2024

demoraesgui commented Sep 17, 2024 • edited Loading

demoraesgui commented Sep 17, 2024 • edited Loading

ZakaryH commented Sep 17, 2024

ZakaryH commented Sep 17, 2024

ZakaryH Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

demoraesgui Sep 18, 2024

Choose a reason for hiding this comment

ZakaryH Sep 18, 2024

Choose a reason for hiding this comment

demoraesgui Sep 18, 2024

Choose a reason for hiding this comment

ZakaryH commented Sep 18, 2024

ZakaryH left a comment

Choose a reason for hiding this comment

demoraesgui commented Sep 18, 2024

demoraesgui commented Sep 10, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Sep 10, 2024 •

edited

Loading

ZakaryH Sep 17, 2024 •

edited

Loading

ZakaryH Sep 17, 2024 •

edited

Loading

demoraesgui Sep 17, 2024 •

edited

Loading

demoraesgui Sep 17, 2024 •

edited

Loading

demoraesgui commented Sep 17, 2024 •

edited

Loading

demoraesgui commented Sep 17, 2024 •

edited

Loading

ZakaryH Sep 17, 2024 •

edited

Loading