-
Notifications
You must be signed in to change notification settings - Fork 23
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
Show multiple types in list view #525
base: main
Are you sure you want to change the base?
Conversation
@wbazant Exciting and much-awaited change! Overall I think this is great. I played around a bit and made one notable tweak: to keep the font color of the scientific name constant (only change size) if it is standalone. Changing both size and color made it harder to recognize as a scientific name. (I didn't touch the location page, but presume that will get redesigned anyway) I also wanted to make some other tweaks:
I do think that the list icons (thumbnail and ">" should be top-justified, so they stay put even if the list entry gets longer. Could you make that change? As for the tag styling, it feels visually a bit loud in the list, but I'd like to first see how these type tags look like on the location page (is that the plan?) before suggesting changes. For testing, a good location is http://localhost:3000/locations/1919726, which in French has a standalone common name, a standalone scientific name, and a name pair. |
Thanks for the tweaks and for pointing out the types without common names are doing something else, I've not been focusing on that. I'd like to go even further and I removed the 'standalone' version of scientific names from TypeName, and I think both the types select and the list gain by that. I've then tried to go further yet and saw how TypeTitle looks without the standalone property, and I think it is better to keep it on location pages for now. I made an issue #530 to track the changes to the location page - I agree the dropdowns can take a lot of space. I don't really know how to improve it - I reckon the badges for wikipedia and USDA link-outs are a bit cluttered, and I'm not sure how else to add the links. Also, there's already something else rendered as a tag there - property access - and overall, I'd be inclined to not try to solve this right now, since the type titles look decent for the locations with just a few types, and they are the majority.
I'll address your other comments with changes, right now if I end up having enough time or later on. |
…h the centre for single-line locations
I've tweaked the height estimation formula a bit. I think there's no way to make it work fully reliably with just a calculation, or at least I don't understand enough about how elements are rendered. So I regrettably propose the solution in which the heights are sometimes a bit wrong. Sometimes we undershoot the calculation and then there's a scroll bar, see http://localhost:3000/list/@55.8273030,-4.2537105,18z . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason "Settings" in the desktop sidepane is gone:
As for the height, I see that it is both sometimes underestimated or overestimated, and the code for the height calculation is ... brittle. Can you explain why this is necessary? What fancy design feature (pre-loading animation, infinite scroll) can we drop so that we can just sit back and let the layout grow to the content?
Some other options:
One thought I had here that's somewhat tangential is that Falling Fruit doesn't have to minimise user friction with the goal of maximising engagement - there are no dollars per eyeball-second involved - and it's not even obvious why we should have the users spend as much time on the site as possible, maybe reminding the user they've just mindlessly scrolled through fifty locations is actually a good thing - so a "Show more" instead of an infinite list should be totally fine from the UX perspective. |
@wbazant Thanks for explaining. Hmm.
For what it is worth, the mobile app list maxes out at 50 locations, period, and no one has ever complained. So a simple "Show more" (or can we get away with a languageless "51–100 (189)") is probably fine. While we are on the topic of scrolling: Right now, if the user scrolls down the list, navigates away and then back (for example clicks on a location and back, or to the map and back), the list is identical but ends up back at the top. They've lost their place in the list, which is annoying if trying to use the list to find interesting locations. I presume the only solution to this is to keep the list from being re-rendered? Does this have any implications here with regards to infinite or paginated scrolling? If we want to also use the list for recent activity, etc, then it seems like pre-calculating height quickly gets out of hand? |
Tweak the styles a bit. Skeletons for unloaded entries (up to five)
I removed the react-window virtualization and made an infinite list with up to five skeleton elements that show there's more content. The main differences are the number of skeleton elements, and the scrollbar size. I started from a 'Show more' button but actually, it wasn't too hard to auto-load and it's a nicer experience! To test the auto-loading, go to somewhere with lots of locations, like Boulder, CO , or change the chunk size in |
@wbazant Hey I don't really miss the infinite scroll and it is nice to be back in control of the layout. At this point, I'd say we're ready to merge. The one funky thing I stumbled across is what happens when a single type is too wide: See http://localhost:3000/list/@40.5694776,-105.0919469,19z if you want a bunch of thornless honey locusts. But I guess the styling as tags sort of requires this behavior? Otherwise, I think, design-wise:
|
- remove right chevron - flex-wrap for TypeNameWrapper - remove styles that didn't do anything - tweak margin around borders
I adjusted the space, removed the right chevron, and set flex-wrap: wrap; on TypeName which also fixes #551. In the wrap case, I couldn't quite get the boundary to align with the words, it does this: |
Hmm, I'm not sure this is possible in CSS, as explained here https://stackoverflow.com/a/37413580. But here is a fiddle to play with: https://jsfiddle.net/me9vrqdL/2/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise it looks fine and could be merged.
Closes #524.
Also closes #551