Mobile/Filter Overflow UX Improvements #503
Conversation
Still needs clear all functionality, various styles and spacing, and overall cleanup
Most of the styling issues are still present, but collapsing/expanding logic should be complete
|
@ukutaht if you're still around, I'd love your thoughts on the filter dropdown implementation - https://dev.vigneshjoglekar.com/plausible.io?period=30d&country=USA&browser=Chrome&source=Hacker+News&browser_version=87.0 - it's not done as far as styling/spacing on mobile goes, but the actual collapsing logic should be all done. (I do still have a chunk of refactoring etc to do, so the code is likely not close to its completed state). |
|
Hey, I'm still around today and thanks for this. I get this issue when testing:
|
|
Yeah I found some other issues, gonna poke around - will let you know when its ready again |
|
Alrighty - it should be working well now, or at least I haven't been able to break it yet. |
|
Thanks @Vigasaurus. I think it behaves correctly, no issues there. I've been looking at the code for about 30mins and I'm still completely confused by it. Can you try to refactor the it to add more clarity? I'll probably go over it as well once you're done. |
|
Thanks @Vigasaurus! I tested it on my phone. I like the new filters dropdown. Takes so much less space. In the custom range picker the font for the year is black. The custom date picker popup goes outside of the screen for me. I wonder if there's a way to center it like the rest of the app?
I get this error when I clear filters if I'm on a custom selected date range. From what I've seen, it doesn't happen when I'm not on custom range.
I wonder if there's a way to make the sticky header take one line only on mobile devices? Right now the second line is half empty only showing the date. Perhaps retitle "Active Filters" to "Filters" on mobiles save space? And maybe replace the actual dates with a calendar icon or something like that on mobiles?
What do you think? |
|
Yeah absolutely, I'm planning on it. Most of the code right now is pretty messy and has a lot of unnecessary/unused stuff and is lacking some pretty vital comments to make stuff clear. My biggest thing here was just to verify functionality :D |
|
@metmarkosaric yeah I did notice the black year there too, I'm pretty sure it's just that I didn't account for the disabled selector (I'll add it). As for centering the popups - I am planning on that for sure. On mobile I intend to make them generally bigger and centered to try to make them more similar to native dropdowns instead of the desktop style. As for the error with the date, I haven't seen that before but I'll definitely try to reproduce it/see if I can find a root cause/if I caused it. As for making it a one-liner, I'm unsure - because right now your view is also a bit compressed since it's missing the down arrow for site switching. I can look into it for sure, but I have a hunch that it'll 1. get very overcrowded and condensed and 2. not play very well with long domain names, and I think that truncating there could cause problems for those with multiple similar domains. |
|
Ok I'll try and test it with logged in state and other domain names when it's on staging. In general the feedback was that sticky header takes too much space on mobile devices so if there are elements we can hide on small screen to make it fit in one line, it's worth considering. |
|
Yeah that makes sense for sure, I just fear that in an effort to save space we may inadvertently remove some of the intuitive nature of Plausible's UI. I'll definitely explore it and see what solutions I can come up with. |
|
@ukutaht Code refactored, rest should be all ready for review. Definitely lmk if you want more cleanup/explanation in @metmarkosaric All the things you mentioned should be fixed, check out the PR main body for details It's still live on my dev server, so let me know if you see any other issues :D |
|
Just tested it and it looks great @Vigasaurus, thanks! We could try and get this onto staging so we can test with different domain names etc and see if it all could fit on one line somehow. |
There was a problem hiding this comment.
I had a question about the function declaration style. Also, all the other React components are defined as classes in this project (functions only for pure components). I'm not a fan of mixing multiple different styles.
Overall this functions well and I'm eager to merge.
On the code level I think the screenclass-hook and recheck variable add unnecessary complexity. I'd like to get rid of them if possible. Will take a look on my machine to see if it's possible
|
Yeah that all makes sense. My biggest reason for using functional here was from how I was able to think up the functionality it needed/the ease in implementation that functional could afford (and useLayoutEffect specifically), but I bet it is possible to do this in a class component. I'll take a look at that and removing the complexity you mentioned too and get it refactored to match 👍 |
- Removes Screenclass hook in favor of simple viewport size storage - Uses class-based component - Removes recheck variable in favor of tri-state wrapped - General formatting & code cleanup
|
nice @Vigasaurus, thanks for this! |
Changes
date undefinederror that Marko found (and cause a major issue on currently deployed prod where if you remove a filter with any of the three fields in the queryfrom, to, date- they get removed no matter what)Tests
Changelog
Documentation