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

Show multiple types in list view #525

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

wbazant
Copy link
Collaborator

@wbazant wbazant commented Oct 18, 2024

Closes #524.

  • opacity 0.5 for types that are not selected, but co-occur with selected types
  • estimate height of elements, which VariableSizeList asks for: I got the initial implementation from the AI assistant and then checked various constant values and tweaked some, it seems to work across a number of cases I checked!
  • a minor refactoring move for ui/ListEntry, which is not actually needed for the PR because I ended up dropping the dependency, but it felt like I made an improvement with it so I ended up keeping the commit

Also closes #551

@ezwelty
Copy link
Collaborator

ezwelty commented Oct 18, 2024

@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:

  • Drop the "unnecessary"
    wrapping CommonName and ScientificName, but this breaks the type select dropdown.
  • Simplify the font size logic and move font size to the parent TypeName, setting non-standalone ScientificName to 90%. This would fix a minor styling issue that the line-height of the parent is larger than the content, which makes the tag padding larger above than below. But then I realized the styling is coupled to the type select dropdown and gave up.

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.

Screenshot 2024-10-18 at 14 38 39

@wbazant
Copy link
Collaborator Author

wbazant commented Oct 19, 2024

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.

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.
I'd not do the location page with tag styling, at least for now. It's fair feedback, the list does pop with a lot of colour. We could tone it down, but some kind of visual separation between the types is necessary, otherwise they start to blend in:
Screenshot from 2024-10-19 08-42-18

I'll address your other comments with changes, right now if I end up having enough time or later on.

@wbazant wbazant marked this pull request as draft October 19, 2024 08:11
@wbazant wbazant marked this pull request as ready for review October 19, 2024 09:10
@wbazant
Copy link
Collaborator Author

wbazant commented Oct 19, 2024

Here's the icons top-justified:
Screenshot from 2024-10-19 10-10-49

I've also just noticed the distance is slightly further from the bottom border when there are more lines, probably shouldn't have undrafted the PR!

@wbazant wbazant marked this pull request as draft October 20, 2024 15:26
@wbazant wbazant marked this pull request as ready for review October 21, 2024 09:39
@wbazant
Copy link
Collaborator Author

wbazant commented Oct 21, 2024

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 .

@ezwelty ezwelty self-requested a review October 24, 2024 10:50
Copy link
Collaborator

@ezwelty ezwelty left a 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:

image

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?

image

image

@wbazant
Copy link
Collaborator Author

wbazant commented Oct 25, 2024

As for the height, I see that it is both sometimes underestimated or overestimated
Eh, it bothered me too, and then I've convinced myself I'm sweating over an issue that doesn't really matter, so instead I had a go at tweaking the formula a bit more. But I also don't like the trade-off where we get a fancy feature that works 95% of the time.

What fancy design feature
The students virtualised the list with react-window, so the list can sometimes appear to have hundreds of elements, but work really fast, since only the visible elements are actually in the DOM, and the scrollbar still shows the user how many elements there are in the list.

Some other options:

  • a "Show more" at the end of say 50 elements, and they would all be in the DOM - hoping almost no one would want to endlessly "show more" because the list stops being interesting
  • a "Show next (51-100 of 189)" at the end of 50 elements, or another form of pagination
  • a homemade infinite list with a homemade "load more if at the end" - lets user not click the "show more" but the scrollbar mysteriously extends so not a very good option
  • a virtualised list with react-render-if-visible - since it uses actual heights but estimates for the scrollbar of the whole list, I understand the scrollbar will be a tiny bit janky

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.

@ezwelty
Copy link
Collaborator

ezwelty commented Oct 25, 2024

@wbazant Thanks for explaining. Hmm.

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.

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)
@wbazant wbazant requested a review from ezwelty October 26, 2024 14:08
@wbazant
Copy link
Collaborator Author

wbazant commented Oct 26, 2024

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 src/redux/listSlice.js.

@ezwelty
Copy link
Collaborator

ezwelty commented Oct 29, 2024

@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:

  • The space below "x meters" to the border is too small and should match the space between the top border and the content.
  • The ">" icon is not vertically aligned with the center of the photo thumbnail, which looks weird to me. Also it has way too much left margin, space that would be better used for content. In fact, I'd actually suggest dropping it since the whole row is a button anyway? The page is much "calmer" without it, and then the narrow-screen issue I highlight above is much less likely. Waddya think?!

 - remove right chevron
 - flex-wrap for TypeNameWrapper
 - remove styles that didn't do anything
 - tweak margin around borders
@wbazant
Copy link
Collaborator Author

wbazant commented Oct 30, 2024

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:
Screenshot from 2024-10-30 09-49-04
Any ideas how to make the background look like just a rectangle around the words? i'd be quite ready to make a 'help wanted' follow up issue with this, I tried a bunch of things and didn't get anywhere.

@ezwelty
Copy link
Collaborator

ezwelty commented Oct 31, 2024

Any ideas how to make the background look like just a rectangle around the words?

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/

Copy link
Collaborator

@ezwelty ezwelty left a 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.

@wbazant
Copy link
Collaborator Author

wbazant commented Oct 31, 2024

All right, the browser is telling me to not try to create tag-looking elements that sometimes take more than a page. So - we're not going in circles with this, we're going up a spiral :) - maybe no tags?

I also removed flex on type names, because it wrapped like this:
Screenshot from 2024-10-31 17-01-04

but now does this:
Screenshot from 2024-10-31 17-12-14

My original intent behind the orange backgrounds was to make the different types look distinct - and also it popped when I've set it, so I like it - but there wasn't any other rationale for it.

Extreme example:
Screenshot from 2024-10-31 17-15-40

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.

Type names in Edit Location sometimes go off the screen Show multiple types in list view
2 participants