GetJobber / atlantis

🔱 Atlantis
https://atlantis.getjobber.com
MIT License
25 stars 30 forks source link

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

Closed demoraesgui closed 5 days ago

demoraesgui commented 1 week ago

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.

Random photo of Atlantis

cloudflare-workers-and-pages[bot] commented 1 week ago

Deploying atlantis with  Cloudflare Pages  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 commented 1 week ago

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 commented 1 week ago

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 commented 6 days ago

@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 commented 6 days ago

@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!

ZakaryH commented 6 days ago

@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 commented 5 days ago

tried it out with a bunch of different prefix content image

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

image

demoraesgui commented 5 days ago

No worries, thanks for great feedback and QA :)