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

fix(refine-modal): include facets from atomic-external #4219

Merged
merged 10 commits into from
Aug 14, 2024

Conversation

louis-bompart
Copy link
Collaborator

@louis-bompart louis-bompart commented Jul 24, 2024

Consider all atomic-search-interface & atomic-external for the facet acquisitions of the refine-modal.

Rough algo

  • If a facet is not in a section, put it in the 'orphan bucket'
  • Sort facet by interface
  • In a same interface, put vertical facet first, horizontal facet second
  • Once all non-orphaned facets are sorted out, sort the orphan facets in DOM order, and add'em.

KIT-3428

Copy link

github-actions bot commented Jul 24, 2024

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

Live demo links

Bundle Size

File Old (kb) New (kb) Change (%)
case-assist 244.1 244.1 0
commerce 341.2 341.2 0
search 414 414 0
insight 391.8 391.8 0
product-listing 306.4 306.4 0
product-recommendation 210.4 210.4 0
recommendation 257.2 257.2 0
ssr 405.1 405.1 0
ssr-commerce 338.5 338.5 0

SSR Progress

Use case SSR (#) CSR (#) Progress (%)
search 39 44 89
recommendation 0 4 0
product-recommendation 0 10 0
product-listing 0 13 0
case-assist 0 6 0
insight 0 27 0
commerce 0 15 0
Detailed logs search : buildInteractiveResult
search : buildInteractiveInstantResult
search : buildInteractiveRecentResult
search : buildInteractiveCitation
search : buildGeneratedAnswer
recommendation : missing SSR support
product-recommendation : missing SSR support
product-listing : missing SSR support
case-assist : missing SSR support
insight : missing SSR support
commerce : missing SSR support

@louis-bompart louis-bompart changed the title fix/KIT 3428 fix(refine-modal): include facets from atomic-external Jul 25, 2024
Copy link
Contributor

@alexprudhomme alexprudhomme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I tested with the facet-manager and also with automatic facets. Two things :

1-The facets in the facet manager seem to be in random order.

   < atomic-layout-section section="facets">
          <atomic-facet field="author" label="facet1"></atomic-facet>
          <atomic-facet-manager>
            <atomic-facet field="author" label="facet10"></atomic-facet>
            <atomic-facet field="author" label="facet11"></atomic-facet>
            <atomic-facet field="author" label="facet12"></atomic-facet>
          </atomic-facet-manager>
        </atomic-layout-section>

first load
image
second load
image

2-The automatic facets are being put at the bottom.

<atomic-layout-section section="facets">
          <atomic-facet field="author" label="facet1"></atomic-facet>
          <atomic-automatic-facet-generator
            desired-count="3"
          ></atomic-automatic-facet-generator>
        </atomic-layout-section>
image

This sorting stuff is so annoying 😢

@louis-bompart
Copy link
Collaborator Author

Looks good, I tested with the facet-manager and also with automatic facets. Two things :

1-The facets in the facet manager seem to be in random order.

   < atomic-layout-section section="facets">
          <atomic-facet field="author" label="facet1"></atomic-facet>
          <atomic-facet-manager>
            <atomic-facet field="author" label="facet10"></atomic-facet>
            <atomic-facet field="author" label="facet11"></atomic-facet>
            <atomic-facet field="author" label="facet12"></atomic-facet>
          </atomic-facet-manager>
        </atomic-layout-section>

first load image second load image

We are having facets on all the same fields, so yeah, suboptimal for the facet manager 😆
Facet manager order facets like they want, then we follow for those

2-The automatic facets are being put at the bottom.

<atomic-layout-section section="facets">
          <atomic-facet field="author" label="facet1"></atomic-facet>
          <atomic-automatic-facet-generator
            desired-count="3"
          ></atomic-automatic-facet-generator>
        </atomic-layout-section>
image

If you put the automatic facet after the static facet, yeah, that make sense 😅

This sorting stuff is so annoying 😢

Complicated indeed, let's go with baby steps, I think it's better than what's currently in place :)

@louis-bompart louis-bompart added this pull request to the merge queue Aug 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 14, 2024
@louis-bompart louis-bompart added this pull request to the merge queue Aug 14, 2024
Merged via the queue into master with commit 107c56b Aug 14, 2024
111 checks passed
@louis-bompart louis-bompart deleted the fix/KIT-3428 branch August 14, 2024 16:11
fpbrault pushed a commit that referenced this pull request Aug 23, 2024
Consider all atomic-search-interface & atomic-external for the facet
acquisitions of the refine-modal.

Rough algo
- If a facet is not in a section, put it in the 'orphan bucket'
- Sort facet by interface
- In a same interface, put vertical facet first, horizontal facet second
- Once all non-orphaned facets are sorted out, sort the orphan facets in
DOM order, and add'em.

KIT-3428

---------

Co-authored-by: Alex Prudhomme <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants