search

coveo / ui-kit

Coveo UI kit repository, home of @coveo/headless, @coveo/atomic, and more.
Apache License 2.0
51 stars 34 forks source link

[BB pr#213] feat(headless): add support for breadcrumbs #213

Closed coveobot closed 3 years ago

coveobot commented 3 years ago

Pull request :twisted_rightwards_arrows: created by @nrawji on 2020-10-26 13:38 Last updated on 2020-11-05 19:24 Original Bitbucket pull request id: 213

Participants:

  • @acote-coveo (reviewer)
  • @ThibodeauJF (reviewer) :heavy_check_mark:
  • @btaillon (reviewer)
  • bitbucket user coveo-anti-throttling_02
  • @jlblais (reviewer)
  • @olamothe (reviewer)
  • @samisayegh (reviewer) :heavy_check_mark:
  • @nrawji

Source: Commit 0050a891eab7 on branch KIT-96-2> Destination: https://bitbucket.org/coveord/ui-kit/commits/4414c017902e on branch master Merge commit: https://github.com/None/commit/517f1441e09f

State: MERGED

feat(headless): finished breadcrumb manager

coveobot commented 3 years ago

@samisayegh commented on 2020-10-26 16:09

Outdated location: line 1 of packages/headless/src/controllers/facets/category-facet/headless-category-facet-utils.ts

Let’s rename these utils files to actions files. e.g. headless-category-facet-actions.ts.

coveobot commented 3 years ago

@samisayegh commented on 2020-10-26 16:11

Outdated location: line 33 of packages/headless/src/controllers/breadcrumb-manager/headless-breadcrumb-manager.ts

For the category facet, wouldn’t mapping generate a separate breadcrumb of every parent vs. generating a single breadcrumb of the path?

coveobot commented 3 years ago

@samisayegh commented on 2020-10-26 16:14

Outdated location: line 36 of packages/headless/src/controllers/breadcrumb-manager/headless-breadcrumb-manager.ts

This function is too long. Let’s extract each facet breadcrumb builder into it’s own function. The getBreadcrumbs function will just be responsible for putting everything together. e.g.

‌

function getBreadcrumbs() {
    return [
       ...getFacetBreadcrumbs(),
       ...getNumericFacetBreadcrumbs()
    ]
}

‌

coveobot commented 3 years ago

@samisayegh commented on 2020-10-26 16:16

Outdated location: line 55 of packages/headless/src/controllers/breadcrumb-manager/headless-breadcrumb-manager.ts

One drawback of concatenating the start and end of the range into the string is it becomes harder for the user of the controller to customize how these values are displayed. How can we make it easy to customize the values without needing to parse the string?

coveobot commented 3 years ago

@samisayegh commented on 2020-10-26 16:23

Outdated location: line 37 of packages/atomic/src/components/atomic-breadcrumb-manager/atomic-breadcrumb-manager.tsx

This method is needed for the facets because they can be reordered, but it should not be necessary for the breadcrumbs.

‌

coveobot commented 3 years ago

@samisayegh commented on 2020-10-26 16:25

Outdated location: line 45 of packages/atomic/src/components/atomic-breadcrumb-manager/atomic-breadcrumb-manager.tsx

The approach is good for an initial implementation. Something to think about though related to the comment below; how would someone customize the breadcrumb display value in the case of e.g. a date facet value?

coveobot commented 3 years ago

@samisayegh commented on 2020-10-26 16:27

Outdated location: line 8 of packages/quantic/package-lock.json

Why was the package-lock for the quantic project affected by the changes?

coveobot commented 3 years ago

@ThibodeauJF commented on 2020-10-26 17:10

Outdated location: line 37 of packages/atomic/src/components/atomic-breadcrumb-manager/atomic-breadcrumb-manager.tsx

yes you subscribe in the initialize method already

coveobot commented 3 years ago

@olamothe commented on 2020-10-26 19:13

Outdated location: line 55 of packages/headless/src/controllers/breadcrumb-manager/headless-breadcrumb-manager.ts

I think that having a single type Breadcrumb for all type of facets won’t work. Probably different types for each type of facet.

It could also return the underlying facet value object underneath, instead of mapping to string ?

As long as they are straightforward to use for the end user.

coveobot commented 3 years ago

@nrawji commented on 2020-10-27 13:28

Outdated location: line 55 of packages/headless/src/controllers/breadcrumb-manager/headless-breadcrumb-manager.ts

The problem with having multiple Breadcumb types is that it now requires the end user to implement some sort of runtime check to see what type of Breadcrumb they’re receiving. We could add a type attribute to the Breadcrumb to make this more obvious but it still would require them to implement some sort of switch statement.

coveobot commented 3 years ago

@ThibodeauJF commented on 2020-10-27 18:44

Outdated location: line 20 of packages/headless/src/controllers/facets/range-facet/numeric-facet/headless-numeric-facet-actions.ts

usually actions aren’t prepended by “headlesss”

coveobot commented 3 years ago

@ThibodeauJF commented on 2020-10-27 18:56

Outdated location: line 17 of packages/headless/src/controllers/facets/range-facet/headless-range-facet-actions.ts

typo here rage → range

coveobot commented 3 years ago

@ThibodeauJF commented on 2020-10-27 19:02

Not really sure I understand why those files exist:

When there is already action files for each of those, isn’t it a bit confusing and pattern breaking?

‌

coveobot commented 3 years ago

@nrawji commented on 2020-10-27 19:05

The intent was that these actions were scoped to the controllers rather than the features, I labelled the files as util files but Sami suggested to rename them action files. Either way since the breadcrumb manager also needs to call the toggleSelect method for each of the facets I had to make a general way to call them outside of the controller.

coveobot commented 3 years ago

@ThibodeauJF commented on 2020-10-27 19:21

I don’t really agree with this… why would these actions only be scoped to the controllers?

Couldn’t some of them be useful to a dev not using the controller but building their own facet? I’d see toggleRangeFacetSelect somehow useful for example, how else would I select a facet using actions, if you don’t export it with the others?

It will make testing easier as well.

I know headless pretty well, but even me im confused as to why there are actions in headless-range-facet-actions & headless-range-facet-actions , both doing facet stuff.

@{557058:ef2198d5-03b4-4d5f-86e7-0a23e51f4f14} do you have a reason why it’s best to keep both separate?

coveobot commented 3 years ago

@nrawji commented on 2020-10-27 19:30

Just to clarify I was the one who initially kept the files separate because I just thought of it as making a utility method that allowed for toggleSelect to be called from both the breadcrumb manager and the individual facet controllers.

Sami just suggested re-naming it from headless-range-facet-utils to headless-range-facet-actions. I do agree that having two separate actions files is a bit too complicated, so I’ll move it into the file inside the features folder unless anyone sees a reason not to.

With that said, to your point about the usefulness of having a toggle-select action, I think in general we have a lot of methods inside controllers that just dispatch 2 or 3 actions in sequence without a lot of other logic. I think in general it makes sense to migrate those to thunk actions when possible.

coveobot commented 3 years ago

@ThibodeauJF commented on 2020-10-27 19:33

Ok perfect then! :ok_hand:

coveobot commented 3 years ago

@olamothe commented on 2020-10-29 18:59

Outdated location: line 44 of packages/headless/src/controllers/breadcrumb-manager/headless-breadcrumb-manager.ts

There’s generic code/pattern between each of those function (getFacetBreadcrumbs, getCategoryFacetBreadcrumbs, getNumericFacetBreadcrumbs) that could be extracted and shared to make the code more concise overall.

coveobot commented 3 years ago

@olamothe commented on 2020-10-29 18:59

Outdated location: line 226 of packages/headless/src/controllers/facets/category-facet/headless-category-facet.test.ts

:see_no_evil:

coveobot commented 3 years ago

@olamothe commented on 2020-10-29 19:01

Outdated location: line 12 of packages/headless/src/features/facets/range-facets/generic/range-facet-utils.ts

Typo ToggleRange

coveobot commented 3 years ago

@nrawji commented on 2020-10-30 14:51

Outdated location: package-lock.json

I think I’m getting weird lock files because I updated to npm 7 I’ll downgrade to npm 6 for now. It looks like they updated how package-lock files are generated in the new release.

coveobot commented 3 years ago

@ThibodeauJF commented on 2020-11-02 21:40

It would be good to have the title of the facet besides every breadcrumb, and to have a Clear all button (deselect all filters)

coveobot commented 3 years ago

@ThibodeauJF commented on 2020-11-02 21:41

Outdated location: line 82 of packages/atomic/src/components/atomic-breadcrumb-manager/atomic-breadcrumb-manager.tsx

I’d put spaces and prevent the last segment of the path to be followed by /

‌

coveobot commented 3 years ago

@ThibodeauJF commented on 2020-11-02 21:42

Outdated location: line 1 of packages/headless/src/controllers/breadcrumb-manager/headless-breadcrumb-manager.test.ts

buildMockSearchAppEngine

coveobot commented 3 years ago

@ThibodeauJF commented on 2020-11-02 21:42

Outdated location: line 22 of packages/headless/src/controllers/breadcrumb-manager/headless-breadcrumb-manager.test.ts

MockEngine<SearchAppState>;

coveobot commented 3 years ago

@samisayegh commented on 2020-11-03 14:15

Outdated location: line 82 of packages/atomic/src/components/atomic-breadcrumb-manager/atomic-breadcrumb-manager.tsx

Another approach to avoid a trailing delimiter could be to join the array on the delimiter. e.g

const path = breadcrumb.path.join('/')

‌

coveobot commented 3 years ago

@samisayegh commented on 2020-11-03 14:20

Outdated location: line 79 of packages/atomic/src/components/atomic-breadcrumb-manager/atomic-breadcrumb-manager.tsx

If this check is needed, then there must be a bug inside headless. If a category facet breadcrumb is returned by the breadcrumb manager, then the breadcrumb must have a path. Otherwise, the value would not be a breadcrumb value.

coveobot commented 3 years ago

@samisayegh commented on 2020-11-03 14:26

Outdated location: line 50 of packages/atomic/src/components/atomic-breadcrumb-manager/atomic-breadcrumb-manager.tsx

Two suggestions regarding the type:

  1. I think we can rename GenericBreadcrumb to just Breadcrumb.
  2. I think we should not export GenericBreadcrumb, but rather four types, one for each kind of facet. This removes the need for developers to manually reconstruct the type, and allows them to leverage intellisense auto-complete. e.g.
type Breadcrumb = ... // not exported

export type FacetBreadcrumb = Breadcrumb<FacetValue>

‌

coveobot commented 3 years ago

@samisayegh commented on 2020-11-03 14:45

Outdated location: line 103 of packages/headless/src/controllers/breadcrumb-manager/headless-breadcrumb-manager.ts

Related to the comment inside Atomic, I think we could prevent push breadcrumbs with empty array paths by checking for that here.

coveobot commented 3 years ago

@samisayegh commented on 2020-11-03 14:54

Outdated location: line 104 of packages/headless/src/controllers/breadcrumb-manager/headless-breadcrumb-manager.ts

Is value needed here? I think the value it contains is not clear, and is there to satisfy the interface. I think we should adjust the interface to just:

interface CategoryFacetBreadcrumb {
    path;
    deselect;
}

‌

coveobot commented 3 years ago

@samisayegh commented on 2020-11-03 14:57

Location: line 144 of packages/headless/src/controllers/breadcrumb-manager/headless-breadcrumb-manager.ts

Shouldn’t the type here be string[]?

‌

coveobot commented 3 years ago

@nrawji commented on 2020-11-03 16:58

Location: line 144 of packages/headless/src/controllers/breadcrumb-manager/headless-breadcrumb-manager.ts

I think its more consistent with the other facets to return the entire value rather than just a string. For example if they want to dispatch deselect on a specific value, rather than the entire path, this allows them to do so.

coveobot commented 3 years ago

@nrawji commented on 2020-11-03 17:11

What do you mean by title? Also I’m not sure if a clear all button should be part of the breadcrumb manger. It seems like it should be it’s own thing that can be used in multiple places depending on the clients needs.

coveobot commented 3 years ago

Bitbucket user coveo-anti-throttling_02 commented on 2020-11-03 17:19

Pull Request Report

PR Title

:white_check_mark: Title follows the conventional commit spec.

Bundle Size

File Compression Old (kb) New (kb) Change (%)
dist/browser/headless.js bundled 286 292.5 2.3
minified 114.3 116.2 1.7
gzipped 32.1 32.5 1.1
dist/browser/headless.esm.js bundled 277.4 283.7 2.3
minified 143 146.5 2.5
gzipped 35.3 35.9 1.6
coveobot commented 3 years ago

@samisayegh commented on 2020-11-03 17:21

@{5a3c40fad62dd77f94582df2} @{5f231ff6a1648700152a2975} , my thinking was to keep the higher order actions next to the controllers and separate from the lower-level actions.

By grouping together, it is now less clear what the differences are. For example. we now export executeToggleCategoryFacetSelect and toggleSelectCategoryFacetValue. The only indication is that one is a thunk. This requires a developer to know that the thunk version of an action is making multiple dispatches, whereas the non-thunk is just toggle selecting the value. I don’t think this is a good thing.

I see three possible ways forward:

  1. Keeping all actions grouped, but no longer exporting the lower level actions.

The issue with this is it restricts developers. If I want to build a facet that allows clicking multiple values, and then clicking a buttn to execute a search, it’s not possible to do with the high-level only.

2. Separating high-level and low-level into dedicated files.

Actions can be exported as separate named exports. There is a clear understanding of what level the developer is operating on. Actions are low-level building blocks. ControllerActions are more opinionated, come with analytics, etc.

export * as FacetActions from '...'
export * as FacetControllerActions from '...'

‌

3. Instead of extracting action sequences from the controllers and importing them, can we build controller instances and use them?

// BreadcrumbManager.ts

const facet = buildFacet(engine, {facetId: id})
const breadcrumb = {
  deselect: facet.toggleSelect(value)
}

I think option 3 will require some refactoring, but will be very clean and encapsulated.

For this PR, I don’t think option 1 is a step in the right direction, which leaves option 2 …

‌

coveobot commented 3 years ago

@nrawji commented on 2020-11-03 18:55

I thought about this, and I think option 2 is probably the best. I think for the end user, actually having an opinionated actions which also dispatch the analytics could be pretty useful. I think for a lot of end users, if they just need to something once, it feels unnatural to create an entire controller to do so. Simply dispatching an action that only does what it says it does feels cleaner, even if under the hood using the controller actually does the same thing.

coveobot commented 3 years ago

@ThibodeauJF commented on 2020-11-03 20:51

I’d agree with Option 2, but it has to be documented/explained correctly, both to devs building headless & users of the lib that won’t know wether to use an Action or a ControllerAction.

coveobot commented 3 years ago

Bitbucket user coveo-anti-throttling_02 commented on 2020-11-03 20:59

Pull Request Report

PR Title

:white_check_mark: Title follows the conventional commit spec.

Bundle Size

File Compression Old (kb) New (kb) Change (%)
dist/browser/headless.js bundled 286 286 0
minified 114.3 114.3 0
gzipped 32.1 32.1 0
dist/browser/headless.esm.js bundled 277.4 277.4 0
minified 143 143 0
gzipped 35.3 35.3 0
coveobot commented 3 years ago

@olamothe commented on 2020-11-04 14:35

Personally, I think #2 is over-engineered.

For now, I’d go with just low level actions, and wait to see, when people start using the library more, if there’s actually a problem with people not correctly using the low level actions.

The “explosions of actions, the amount of types and function we expose” can also be a real problem. Because then as a user of the library, when you start importing an action, you have A TON of choice instead of just a select few. And having choice just for the sake of having choice is not always a good thing, and also bloats the reference and documentation we will need to expose.

‌

#3 seems much better. If we can use controller as building blocks for other controllers (composing them together like that), then it’s a very strong sign that our stuff is well designed, IMO.

coveobot commented 3 years ago

@samisayegh approved :heavy_check_mark: the pull request on 2020-11-04 14:41

coveobot commented 3 years ago

@samisayegh commented on 2020-11-04 14:43

Location: line 69 of packages/headless/src/index.ts

Let’s hold off exporting the Controller actions for now. I prefer if we wait until we know more about how devs will use headless.

coveobot commented 3 years ago

@ThibodeauJF approved :heavy_check_mark: the pull request on 2020-11-04 15:37

coveobot commented 3 years ago

Bitbucket user coveo-anti-throttling_02 commented on 2020-11-05 19:22

Pull Request Report

PR Title

:white_check_mark: Title follows the conventional commit spec.

Bundle Size

File Compression Old (kb) New (kb) Change (%)
dist/browser/headless.js bundled 303.4 311 2.5
minified 122.1 124.3 1.9
gzipped 33.3 33.8 1.3
dist/browser/headless.esm.js bundled 293.9 301.4 2.5
minified 153.8 157.9 2.7
gzipped 36.7 37.3 1.6