-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Adds auto-suggestions + validation for manual filters #1089
Conversation
Still needs: - Form structure on filter modal - Edit filter button - Filter dropdown UI improvement - Filter modal mount data collection - Tests - Potentially negating other filters
Still needs: - Tests - Potentially negating other filters - Potentially some code cleanup
- Makes edit buttons full-length & properly sized - Adds remove filter button in edit menu
Still needs - countries and screen into new format - re-style dropdown and background - drop debounce time - tests
Still needs: - tests - goals added as filter
…ytics into manual-filters
This is pretty much ready to go, I'll likely sprinkle some comments and cleanup across the React code, and fix the dialyzer-found issue, but otherwise this should be done as far as user-facing stuff goes. |
Great addition, this makes the filters so much more usable! Looking at the code, I'm always a bit wary about hand-rolling complex JS elements like a search-select box. Especially for handling keyboard input, highlight behaviour, accessibility, etc. I think the UX could be improved. Annoyances I ran into:
I would suggest basing the dropdown style off Tailwind UI which we've used extensively in other areas. They have styled dropdown components integrated with Headless UI to handle some of the nitty-gritty of select box behaviour. We can get a lot of nice things for free from Headless UI while still hand-rolling the async search behaviour. @Vigasaurus if you think that's a reasonable idea, I can email you the code for a basic select box from Tailwind UI. What do you think? |
Makes sense, and yeah I think a down-chevron there could make it good and clear. I'm not sure I understand what you mean by the grey background though - you mean on hover, or for the input? I just took the existing styles from the datepicker for the hover, and for the signup form for the input.
Yeah whoops - that's just an oversight heh
Yeah that makes sense - I had done it the other way to help the ambiguity if both were being selected, but looking at some other uses, that doesn't seem to the be norm and probably more confusing than helpful
So you mean it should retain its suggestions while typing/loading? Or just show a mostly empty area with a loader?
Yeah I quite like the spinner being in the search box on the side, nice and cleanly - but then that poses a different question for above - re: dropdown sticking around.
Yeah that makes sense
Yeah if you want to send that over that'd be cool - I quite like a lot of their stuff. |
Yeah I mean the actual input element. I know we have some of these input elements with grey background still around. But I've been moving towards more standard looking input elements like the ones on plausible.io/register |
Ah okay makes sense, I think I took the style from /login - anyways, if you just want to send over the Tailwind UI component when you get a chance, I'll get all the changes sorted out in the coming days. |
…and the globbing we're used to) for path-based filters (plausible#1067)" This reverts commit b6eeb40.
@Vigasaurus FYI, I've reverted the previous PR from |
* Support IN filter expression * Update clickhouse_ecto
* WIP * Implement graph with Chart.js v3
* update js deps * Update development command Co-authored-by: happysalada <[email protected]>
Looks good to me! At 84.8kb minified, react-select is almost as big as react-dom itself. Any way we could use a lightweight alternative that doesn't bloat our JS bundle as much? I suggested react-select-search earlier because at 9.2 kb minified it's about 10x lighter. |
Yeah I gave With To me, the UX that the alternatives provided vs react-select honestly warranted the larger size of the dep, because as you said, rolling it ourselves will almost always be bad for accessibility, and I think the lack of control on the others makes them really hard to use in this use case (or at least use well). Let me know what you think/if you think we should use the alternative anyways - but really I don't think sacrificing a pretty big chunk of UX for a bundle size reduction is that worth it (and we're still under 1MB not gzipped, and at about 260kb gzipped, anyways 🤷♂️) |
Just needs some additional testing + potentially code cleanup
Thanks for the thorough explanation @Vigasaurus . It all makes sense and I agree that the UX issues with alternative packages seem pretty bad. I have never found a search-select package that doesn't have its problems. Not sure why it's such a difficult component to build (not saying I would do a better job, just that I don't understand what makes it more complex than routing for example). Tree shaking is great, I'll run it locally to see what the real bundle increase is. Another point is that the dashboard code is big enough now where I think code-splitting is warranted. Probably <10% people will open up the manual filter modal so as long as we start doing code splitting at some point we can minimize the impact of react-select for most users. |
@ukutahts sounds good! Yeah search-select is always rough - I really hope headless UI makes a select-search at some point, I have to imagine their implementation would be nice. And yeah I think code splitting for all the modals down the line could be beneficial - having all of them have a slight load is probably not a big deal. Anyways, this is pretty much at a finished state and ready for review now, I'll resolve the conflicts shortly though. |
Closing in favor of #1121 |
Changes
As per discussions in #1067
I probably should've worked on this is a different branch to keep the diffs normal, but eh, it's just built purely on top of #1067
Tests
Changelog
Documentation