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

Refine places display #2581

Merged
merged 32 commits into from Sep 26, 2022
Merged

Refine places display #2581

merged 32 commits into from Sep 26, 2022

Conversation

tiltec
Copy link
Member

@tiltec tiltec commented Sep 15, 2022

Branch deployment URL: https://refine-places-display.dev.karrot.world

Add a new "place gallery" page to have a nice display of all places with additional information. The sidenav place list will only contain favorite places that users marked themselves (with the 'star' button on the place page).

Code-wise, I refactored a bit, mostly by merging three components SidenavPlaces, SidenavPlacesUI and PlaceList. Hence quite some removed lines in the PR.

New place gallery

image

gallery.webm

Closes #2559

@codecov
Copy link

codecov bot commented Sep 15, 2022

Codecov Report

Merging #2581 (4252ab9) into master (cf22b82) will increase coverage by 0.80%.
The diff coverage is 96.27%.

❗ Current head 4252ab9 differs from pull request most recent head 8f0521f. Consider uploading reports for the commit 8f0521f to get more accurate results

@@            Coverage Diff             @@
##           master    #2581      +/-   ##
==========================================
+ Coverage   76.76%   77.57%   +0.80%     
==========================================
  Files         359      354       -5     
  Lines       37010    36126     -884     
  Branches     1825     1793      -32     
==========================================
- Hits        28410    28023     -387     
+ Misses       8600     8103     -497     
Impacted Files Coverage Δ
src/activities/pages/GroupActivities.vue 95.94% <ø> (-1.39%) ⬇️
src/places/helpers.js 94.11% <ø> (-0.41%) ⬇️
src/places/pages/Layout.vue 93.24% <0.00%> (ø)
src/places/pages/Places.vue 96.25% <96.14%> (+96.25%) ⬆️
src/sidenav/components/SidenavPlaces.vue 97.69% <97.43%> (-2.31%) ⬇️
src/group/services.js 88.62% <100.00%> (-0.75%) ⬇️
src/maps/components/GroupMap.vue 96.95% <100.00%> (ø)
src/utils/datastore/helpers.js 71.24% <0.00%> (-4.98%) ⬇️
src/utils/utils.js 85.31% <0.00%> (-3.39%) ⬇️
... and 30 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tiltec tiltec marked this pull request as ready for review September 16, 2022 22:18
@tiltec
Copy link
Member Author

tiltec commented Sep 17, 2022

Since I now treat 'archived' as yet another status, maybe it's too easy to see archived stores? But I would leave it and see if users tell us...

@nicksellen
Copy link
Member

Great! I think it's a really nice improvement :) Elevating places...

A few things off the top of my head before diving into the code:

Would be nice to be able to click the whole box, like in the group gallery.

Maybe can include the place image (the random stuff) in the card? ... and I imagine one day we will have place images.

Add to favourites from the card?

Screenshot_2022-09-19_22-49-23

When there are no activities, or favourite icon, the whitespace looks a bit spacious, could a) make it collapse when there is no "status" row, b) move the status things to the bottom of the card, c) something else, d) nothing.

Screenshot_2022-09-19_22-50-50

Could not show "Unspecified" as the place type if that's the value set... sort of special case...

Screenshot_2022-09-19_22-36-47

I would go for a + button on the top toolbar, rather than this offer-style plus button (I think the offers page would be better off with that too)... so we move towards a more consistent UI/UX.... and I think the + button is better, or, more likely to be able to be standard....

Screenshot_2022-09-19_22-38-43

I think these would look better aligned left, rather than justified. At least that's how they are on the activities page. Filters on the left, buttons on the right. Here I am stuck in the middle with you. Oops went off piste there.

Something funny happens with the activity counts. Looking at the network tab, I think the intention is to fetch activity list with page size of 1200, and do the counts from there. Sometimes it does that, sometimes it loads 10 at a time (and makes lots of requests). And the number in the UI flashes from a low number to a high number... so not sure what's going on. Maybe two things are fighting to set the page size (the page size isn't part of the query key, so I think that is quite likely to be the case somewhere...).

One idea would be to include it as part of the place status data, to add an upcoming_activity_count or similar. Not needed right now unless motivation is present :)

I wonder if for some groups it becomes harder to find the places, as they are no longer in a big list to compare... I guess we can find out...

I wondered at some point if using the QTable component could be a good standard, as it has a grid style mode too --> https://quasar.dev/vue-components/table#grid-style ... and then could offer users the choice of which view they prefer. Again, not needed for now, unless motivation went that way...

Copy link
Member

@nicksellen nicksellen left a comment

Choose a reason for hiding this comment

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

Really nice treat to have :) ⭐

I'll approve it, but some of the suggested changes might be nice to have before merge :)

src/locales/locale-en.json Outdated Show resolved Hide resolved
@@ -183,7 +183,7 @@ export default {
return { opacity: this.showOverlay ? 0.5 : 1 }
},
placesWithLocation () {
return this.places.filter(hasLocation)
return this.places.filter(place => place.status === 'active').filter(hasLocation)
Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, I notice now the sidenav map doesn't keep up to date with the filtered places in the list...

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 kind of ignored the sidenav map... I guess it could also just show the subscribed places, but then it would also be nice to show all places on demand. Leave it for now?

src/places/pages/Places.spec.js Outdated Show resolved Hide resolved
src/places/pages/Places.vue Outdated Show resolved Hide resolved
src/places/pages/Places.vue Outdated Show resolved Hide resolved
src/sidenav/components/SidenavPlaces.vue Show resolved Hide resolved
@tiltec
Copy link
Member Author

tiltec commented Sep 23, 2022

Coming back to this :) I think I addressed most of the points you mentioned, here are the others:

Maybe can include the place image (the random stuff) in the card? ... and I imagine one day we will have place images.

I couldn't figure out a nice design that doesn't take too much space.. also, the RandomArt for places seems less nice than those for groups, too busy and not that different from each other. So, I would pass on this for now...

Could not show "Unspecified" as the place type if that's the value set... sort of special case...

I left it in to have one special case less, maybe it's a motivation to set custom place types ;)

@tiltec tiltec merged commit 919a300 into master Sep 26, 2022
@tiltec tiltec deleted the refine-places-display branch September 27, 2022 16:30
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.

Can't use 'back' button to navigate back from place wall/activities
2 participants