feat(responsive-filters): added examples for vanilla and RISH #417

aymeric-giraudet · 2022-11-02T11:07:43Z

FX-1880

Here are the code samples for vanilla and RISH for the incoming responsive filtering guide.

As in the RFC, I used Bootstrap for vanilla and Reach UI for RISH.

To circumvent conditional rendering in RISH I restore uiState on unmount and instead of using virtual versions of each widget I just render filters twice when the panel is open. This felt like the simplest way possible although it's not the best performance wise.

Vanilla preview

React preview

Haroenv

lgtm

instantsearch.js/responsive-filtering/src/responsiveFilters.js

sarahdayan · 2022-11-07T16:23:27Z

The rangeInput has a problem that makes the experience not responsive at the breakpoint just before moving to dialog mode, and in the modal:

Also, the dialog should have an explicit "close" button. Bootstrap modals provide it.

aymeric-giraudet · 2022-11-07T16:28:03Z

Oh indeed @sarahdayan, rangeInput is very wide on Firefox 😅
I'll pick something else that plays more nicely on our default CSS theme.

sarahdayan

I noticed a few remaining visual issues in addition to the ones I mentioned inline:

The Filters button isn't aligned with the Featured select. It also doesn't have the same border-radius. I recommend checking the Satellite website to ensure the styles here are compliant.
The action buttons at the bottom of the responsive panel look a bit small to me, i'd expect them to be the same size as the Filters button and Featured select. Also, same comment about the border-radius.

instantsearch.js/responsive-filtering/src/app.js

sarahdayan · 2022-11-14T12:17:03Z

instantsearch.js/responsive-filtering/manifest.webmanifest

+  "short_name": "instantsearch.js-app",
+  "name": "instantsearch.js-app Sample",


These should reflect that this app is about responsive filtering.

I don't recall what are the recommended lengths for each attribute though, I'll let you verify @aymeric-giraudet.

short_name's recommended max length is 12 😬
name's is 45 though so I was able to put more stuff

sarahdayan · 2022-11-14T12:25:03Z

instantsearch.js/responsive-filtering/index.html

+        <div id="searchbox"></div>
+
+        <div class="search-panel__filters">
+          <div class="responsive-filters__desktop" id="filters-desktop">


If we're going with BEM classes, then the __desktop part should be a modifier, not an element, because this <div> isn't nested within a responsive-filters block. Same goes for its sibling .responsive-filters__mobile.

Also, do we really need this extra element? Can't we reuse .search-panel__filters?

I used the ID instead, but on React I made it as a block by itself

instantsearch.js/responsive-filtering/index.html

sarahdayan · 2022-11-14T12:55:11Z

instantsearch.js/responsive-filtering/src/responsiveFilters.js

+  document.getElementById('open-filters').addEventListener('click', () => {
+    filtersModal.show();
+    modalBody.append(...filtersDesktop.childNodes);
+  });
+
+  document.getElementById('apply-filters').addEventListener('click', () => {
+    filtersModal.hide();
+    filtersDesktop.append(...modalBody.childNodes);
+  });
+
+  document
+    .getElementById('filters-modal')
+    .addEventListener('hidden.bs.modal', () => {
+      filtersDesktop.append(...modalBody.childNodes);
+    });


I'm curious, why did you decide to copy the nodes from the desktop filters container to the mobile one (and back) instead of having two sets of InstantSearch widgets (one in each container) and just hide the unwanted onew with CSS?

It's easy to do in vanilla as opposed to React, renders just once and might take less code, at least in index.html.

react-instantsearch-hooks/responsive-filtering/src/ResponsiveFilters/Filters.tsx

Co-authored-by: Sarah Dayan <5370675+sarahdayan@users.noreply.github.com>

sarahdayan · 2022-11-18T13:07:24Z

I'm still seeing a few issues visually on the vanilla JavaScript demo, which I believe this is caused by Bootstrap's global CSS. Is there a way for us to easily include just the modal CSS? Or could we maybe reuse the custom CSS you used in the React demo?

A few other things:

The blue buttons don't have an hover state, can we add one (same as Satellite's)?
The Filters button's SVG icon is vut off at the bottom
The modal on the React demo doesn't cover the whole screen
The text of the blue buttons seems bigger than the one of the white buttons

aymeric-giraudet · 2022-11-21T13:29:00Z

@sarahdayan
The icon is supposed to be a bit cut off at the bottom, I took it from the e-commerce example :

Looks the same on Firefox too for me now.

I also added hover and active states to the buttons and reduced the font size to match that of current refinements.

Regarding bootstrap's CSS reboot, there's no way to opt out when importing from their CDN, and I'd rather not install from npm as builders do not necessarily use it and there could be problems with different bundlers.
There was an issue with the fact that we use a div with container which already exists in Bootstrap, apart from that I see some on highlighting which I overrode. Is there any other ?

sarahdayan · 2022-11-21T15:17:58Z

@aymeric-giraudet That's not what i meant by "cut-off", it looks like this on the React example:

I'm on Forefox, this may be different on Chrome.

sarahdayan · 2022-11-21T15:21:23Z

@aymeric-giraudet For Bootstrap, could we maybe copy the CSS code for the modal component and paste it into a local CSS file?

seafoox

Overall looks great, added a few suggestions which I hope we're be able to iterate in order to provide a superior/more user friendly mobile experience by default.

seafoox · 2022-11-21T15:53:40Z

react-instantsearch-hooks/responsive-filtering/src/App.css

+}
+
+.responsive-filters-modal {
+  width: 100vw;


Could we make the panel 80% width by default. This way end-user can get the instant feedback offered by the InstantSearch experience. In terms of behaviour, end-users can close the panel by either clicking on the X button, or in the void space on the left.

Here the mockup we share as part of our Ecom UI Design Kit

TBH I'm still not sold on this pattern. If we want to see the results in the back, I'd rather leave some space at the top of the dialog (like with a native dialog, see picture) because we have more vertical than horizontal space at our disposal.

We'll need Katie's help here if we decide to go to that level of detail for the v1 of the guide and its associated guide. I suggest we discuss it again once I've produced the acceptance criteria.

react-instantsearch-hooks/responsive-filtering/src/ResponsiveFilters/Filters.tsx

react-instantsearch-hooks/responsive-filtering/src/App.css

react-instantsearch-hooks/responsive-filtering/src/ResponsiveFilters/Filters.tsx

aymeric-giraudet added 2 commits November 2, 2022 12:01

feat(responsive-filtering): added instantsearch.js example

7efc20f

feat(responsive-filtering): added react-instantsearch-hooks example

8b214fa

aymeric-giraudet requested review from a team, FabienMotte and sarahdayan and removed request for a team November 2, 2022 11:07

aymeric-giraudet added 2 commits November 3, 2022 09:44

update readme

cb5d4c9

make react example work on codesandbox

d43ec18

Haroenv approved these changes Nov 7, 2022

View reviewed changes

instantsearch.js/responsive-filtering/src/responsiveFilters.js Outdated Show resolved Hide resolved

address comments

555fdbf

sarahdayan requested changes Nov 14, 2022

View reviewed changes

aymeric-giraudet and others added 5 commits November 17, 2022 16:52

CSS comments taken into account

5cb5529

nb-hits to hits-count

b2f1fff

merge imports into one

453c45d

Apply suggestions from code review

b874694

Co-authored-by: Sarah Dayan <5370675+sarahdayan@users.noreply.github.com>

update webmanifest

5c2c7a2

aymeric-giraudet force-pushed the feat/responsive-filters branch from ac1ab77 to 5c2c7a2 Compare November 17, 2022 16:25

aymeric-giraudet added 2 commits November 21, 2022 14:43

Merge branch 'master' into feat/responsive-filters

ddd4d5c

fix styles

6c8ce3d

seafoox self-requested a review November 21, 2022 15:50

fix filter icon in React as well

ed89836

seafoox reviewed Nov 21, 2022

View reviewed changes

rework styles for mobile, add current refinements and collapsible panel

d3a2e39

FabienMotte approved these changes Nov 23, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(responsive-filters): added examples for vanilla and RISH #417

feat(responsive-filters): added examples for vanilla and RISH #417

aymeric-giraudet commented Nov 2, 2022 •

edited

Haroenv left a comment

sarahdayan commented Nov 7, 2022

aymeric-giraudet commented Nov 7, 2022

sarahdayan left a comment

sarahdayan Nov 14, 2022

aymeric-giraudet Nov 17, 2022

sarahdayan Nov 14, 2022

aymeric-giraudet Nov 17, 2022

sarahdayan Nov 14, 2022

aymeric-giraudet Nov 17, 2022

sarahdayan commented Nov 18, 2022

aymeric-giraudet commented Nov 21, 2022

sarahdayan commented Nov 21, 2022

sarahdayan commented Nov 21, 2022

seafoox left a comment

seafoox Nov 21, 2022

sarahdayan Nov 22, 2022

seafoox Nov 25, 2022

		"short_name": "instantsearch.js-app",
		"name": "instantsearch.js-app Sample",

feat(responsive-filters): added examples for vanilla and RISH #417

Are you sure you want to change the base?

feat(responsive-filters): added examples for vanilla and RISH #417

Conversation

aymeric-giraudet commented Nov 2, 2022 • edited

Haroenv left a comment

Choose a reason for hiding this comment

sarahdayan commented Nov 7, 2022

aymeric-giraudet commented Nov 7, 2022

sarahdayan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahdayan commented Nov 18, 2022

aymeric-giraudet commented Nov 21, 2022

sarahdayan commented Nov 21, 2022

sarahdayan commented Nov 21, 2022

seafoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aymeric-giraudet commented Nov 2, 2022 •

edited