feat(components): Separate Chip from Chips Again

ZakaryH commented 2 months ago

Motivations

a fresh, new take on: https://github.com/GetJobber/atlantis/pull/1694

merging/rebasing was a headache with how much has changed since that PR was made. given it consists of 2 commits, I decided cherry picking them onto a fresh branch was the way to go.

Changes

Please refer to the original linked PR for details.

Added

Changed

Deprecated

Removed

Fixed

Security

Testing

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 2 months ago

Deploying atlantis with Cloudflare Pages

Latest commit:	`29b0265`
Status:	✅ Deploy successful!
Preview URL:	https://ceee43e1.atlantis.pages.dev
Branch Preview URL:	https://job-79340-job-80485-actually-cfh1.atlantis.pages.dev

View logs

github-actions[bot] commented 2 months ago

Published Pre-release for 29b026521d1ed7f68b70f5cc90774c6ee933f9f2 with versions:

  - @jobber/components@5.27.2-JOB-79340-29b0265.13+29b02652

To install the new version(s) for Web run:

npm install @jobber/components@5.27.2-JOB-79340-29b0265.13+29b02652

ZakaryH commented 2 months ago

@chris-at-jobber I can't approve this PR because I made it 😅

I was going to approve all the changes that weren't by me, and then ask you for a quick look at the last few commits I've made which are all CSS updates

would you be down to approve this PR? I used the prerelease in product and checked out a handful of existing usages. they all were working as expected.

chris-at-jobber commented 2 months ago

I'll pull it down and do a QA/design review, but I would feel significantly more comfortable with another eng at least giving a cursory glance over things 😅

MichaelParadis commented 2 months ago

Some nice to do general cleanup (can be done as a followup PR): I think this error message should be updated to tell consumers to use Chip from the @jobber/components/Chip / @jobber/components to use the Chip separately.

Some stuff that can be moved to another PR: There is probably some stuff you could do with the exports here to link the Chip from the main Chip folder instead and remove the file entirely since the "empty" chip component isn't exported / used when just importing from @jobber/components based on this.

ZakaryH commented 2 months ago

@MichaelParadis

Some nice to do general cleanup (can be done as a followup PR): I think this error message should be updated to tell consumers to use Chip from the @jobber/components/Chip / @jobber/components to use the Chip separately.

that error only appears if you use the component outside of this context, so we'd need to switch up the way it gets triggered. arguably it would be in addition to the warning about usage outside of the component rather than replacing it because it's still correct that you cannot use the component outside of Chips

furthermore, it's a whole bundle of complexity to determine if they should be using standalone Chip over Chips/Chip. there are things you cannot yet do with standalone Chip that you would need to use Chips/Chip for such as having a prefix. so the error would need to check which props are being passed in, and if those props are all supported by standalone Chip, then yeah we could say please use Chip. however that's getting pretty complicated imo.

Some stuff that can be moved to another PR: There is probably some stuff you could do with the exports here to link the Chip from the main Chip folder instead and remove the file entirely since the "empty" chip component isn't exported / used when just importing from @jobber/components based on this.

sadly we cannot do that yet. the Chips/Chip is still being used. existing implementations all do import { Chip, Chips } from "@jobber/components/Chips" so those would break/need to be updated - making that a breaking change.

instances of the component being used aside, Chips/Chip is still used internally to pass props along

when we do

import { Chip, Chips } from "@jobber/components/Chips";
...

return (
  <Chips type="multiselect">
    <Chip label="whatever" prefix="checkmark"/>
  </Chips>
);

what happens inside the InternalChipMultiSelect.tsx is

      {React.Children.map(children, chip => {
        const isChipActive = isChipSelected(chip.props.value);
        ...

where what happens is that we only use the props off the children. we do not render it directly. so if you do

import { Chip, Chips } from "@jobber/components";

<Chips>
  <Chip role="combobox">
    <Chip.Prefix>
        ...
    </Chip.Prefix>
  </Chip>
</Chips>

as mentioned above this won't work. you won't get that role nor the Prefix. Chips doesn't support the full API of standalone Chip. we'd need to overhaul how Chips gets data from children, and renders them in order to fully support Chip. even that isn't super simple either, if we want to avoid coupling with Chip and Chips - we might not want to just add a Provider to Chips and use the Chips context in Chip.

once that work was completed, then we would be able to deprecate Chips/Chip and give the guidance to use standalone Chip going forward

TL;DR - we can't get rid of Chips/Chip yet since standalone Chip isn't a full replacement, and that would be a breaking change 😅

ZakaryH commented 2 months ago

undoing one of the things we did in the initial PR https://github.com/GetJobber/atlantis/pull/1694/files#r1430475459

The original Chip implementation didn't do this, so we shouldn't either.

in reference to the tabIndex = 0

we didn't have to do it before because the single select and multi select versions of Chips are actually inputs, and so we want to leverage their built in behavior. the dismissible version however is a div and we did need to pass in tabIndex=0

then for singular Chips I think we do want this to default to being focusable via tabIndex=0. if we don't do that, then when it's a div and we don't have an onClick we can't focus it and won't be able to see things like the truncation tooltip unless we hover it

GetJobber / atlantis