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

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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

aymeric-giraudet
Copy link
Member

@aymeric-giraudet aymeric-giraudet commented Nov 2, 2022

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

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

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

lgtm

@sarahdayan
Copy link
Member

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

Capture d’écran 2022-11-07 à 17 20 18
Capture d’écran 2022-11-07 à 17 20 41

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

@aymeric-giraudet
Copy link
Member Author

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

Copy link
Member

@sarahdayan sarahdayan left a comment

Choose a reason for hiding this comment

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

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.

Capture d’écran 2022-11-14 à 13 35 57
Capture d’écran 2022-11-14 à 13 36 21

Comment on lines 2 to 3
"short_name": "instantsearch.js-app",
"name": "instantsearch.js-app Sample",
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

<div id="searchbox"></div>

<div class="search-panel__filters">
<div class="responsive-filters__desktop" id="filters-desktop">
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

instantsearch.js/responsive-filtering/index.html Outdated Show resolved Hide resolved
instantsearch.js/responsive-filtering/index.html Outdated Show resolved Hide resolved
instantsearch.js/responsive-filtering/index.html Outdated Show resolved Hide resolved
Comment on lines +22 to +36
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);
});
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@sarahdayan
Copy link
Member

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
Copy link
Member Author

@sarahdayan
The icon is supposed to be a bit cut off at the bottom, I took it from the e-commerce example :
image
Looks the same on Firefox too for me now.
image

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
Copy link
Member

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

Capture d’écran 2022-11-21 à 16 17 14

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

@sarahdayan
Copy link
Member

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

@seafoox seafoox self-requested a review November 21, 2022 15:50
Copy link
Member

@seafoox seafoox left a comment

Choose a reason for hiding this comment

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

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.

}

.responsive-filters-modal {
width: 100vw;
Copy link
Member

Choose a reason for hiding this comment

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

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 image

Copy link
Member

Choose a reason for hiding this comment

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

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.

image

Copy link
Member

Choose a reason for hiding this comment

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

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.

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.

5 participants