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

Implement new Vue Virtual Grid #468

Merged
merged 3 commits into from
Oct 12, 2020
Merged

Conversation

Mikescops
Copy link
Member

This implement the new virtual grid lib I created: https://www.npmjs.com/package/vue-virtual-grid

Also fast forward #125

And finally fix a bug with title rendering and the new sidebar.

Please take time to QA this to make sure there is no regression I couldn't catch.

Preview of it:

image

and:

image

@Mikescops Mikescops added 3. to review Waiting for reviews feature: timeline Related to the timeline section labels Oct 7, 2020
Copy link
Member Author

@Mikescops Mikescops left a comment

Choose a reason for hiding this comment

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

I found few bugs:

  • (not related to this PR) XML parsing has an issue when there is no images
  • when resizing the window the computation goes wrong for full width items (this has to be fixed in the virtual grid lib)
  • the viewer is locked to folders once again (maybe we will also have to edit the virtual grid lib to manually trigger the next batch of data)

@Mikescops
Copy link
Member Author

* when resizing the window the computation goes wrong for full width items (this has to be fixed in the virtual grid lib)

* the viewer is locked to folders once again (maybe we will also have to edit the virtual grid lib to manually trigger the next batch of data)

Both issues are now fixed + the ones reported by @skjnldsv

@jancborchardt if you can give a try

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Very nice! Only some small feedback:

  • With the new date headings, the "Your photos" heading is not necessary anymore and can be cut :) But make sure there’s some nice whitespace on top of the first date heading
  • The date headings could do with some more whitespace above them, let’s say double of what’s there now (so 72px)
  • I get this strange preview glitch when scrolling (in Firefox) where already loaded pictures fall back to displaying only the filetype icon:
    Photos scroll loading glitch

@Mikescops
Copy link
Member Author

I have the same glitches and I'm not sure why they are there, and it seems somehow that the old grid what already doing that but less visible.

I'll try to find why and fix the other spacing issues.

@Mikescops Mikescops force-pushed the feature/implement-vue-virtual-grid branch from 38ea86a to 1eca940 Compare October 8, 2020 20:02
@Mikescops
Copy link
Member Author

@jancborchardt it's fixed 😄
thanks @skjnldsv

@Mikescops Mikescops force-pushed the feature/implement-vue-virtual-grid branch from 1eca940 to 9f5e274 Compare October 8, 2020 20:04
@skjnldsv
Copy link
Member

skjnldsv commented Oct 9, 2020

it's fixed

Congrats! Was not that easy :p

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

  • Header Navigation is missing when there is pictures (present when there is none)
  • Loading indicator is missing
  • Grid sizes config is not used anymore
  • See inline comments too :)

src/components/SeparatorVirtualGrid.vue Show resolved Hide resolved
src/components/SeparatorVirtualGrid.vue Outdated Show resolved Hide resolved
src/views/Tags.vue Show resolved Hide resolved
src/views/Timeline.vue Outdated Show resolved Hide resolved
src/views/Timeline.vue Outdated Show resolved Hide resolved
src/views/Timeline.vue Outdated Show resolved Hide resolved
@skjnldsv skjnldsv added this to the Nextcloud 21 milestone Oct 12, 2020
@skjnldsv skjnldsv linked an issue Oct 12, 2020 that may be closed by this pull request
@Mikescops
Copy link
Member Author

Mikescops commented Oct 12, 2020

Header Navigation is missing when there is pictures (present when there is none)

@jancborchardt asked to remove it when there is pictures

Loading indicator is missing

I'm not sure how we can re-add the loading indicator, the lib is not ready for this, I think we can open an issue on the lib and fix this later, I'm not convinced it's a blocker

Grid sizes config is not used anymore

As answered in the small comments, gridconfig is now handled on the lib side. It is configurable through functions passed to the grid component. In my opinion we can remove it and set the batch size with a param.

@Mikescops
Copy link
Member Author

I have fixed all suggested issues except the loading icon when waiting for next batch.
For this one I created an issue on the lib side: Mikescops/vue-virtual-grid#8
If it's a non blocking regression we can move forward here.

@jancborchardt
Copy link
Member

jancborchardt commented Oct 12, 2020

Nice! Only found little issues:

  • .grid-container has a white background set which shows in dark mode. It doesn’t actually need any background set? (If it does then we should use variables – but I really don’t think the background needs to be there?)
  • There’s a bit too much space on the top. It’s the padding-top of the heading plus the 66px of padding on the grid-container. I’d just remove the 66px top on the grid container, for any form factor. The padding of the heading is enough :)
  • The bottom of the photo list has very little space, I’d go for something like 256px down there to signify the list is over.
    image

@Mikescops
Copy link
Member Author

@jancborchardt nice catch! done

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks good design-wise! :) Nice!

@Mikescops
Copy link
Member Author

@skjnldsv can I let you merge this?

Signed-off-by: Corentin Mors <corentin.mors@dashlane.com>
Signed-off-by: Corentin Mors <corentin.mors@dashlane.com>
Signed-off-by: Corentin Mors <corentin.mors@dashlane.com>
@skjnldsv skjnldsv force-pushed the feature/implement-vue-virtual-grid branch from 267ac59 to 39b85f9 Compare October 12, 2020 16:30
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 12, 2020
@skjnldsv skjnldsv merged commit 1c1dbf7 into master Oct 12, 2020
@skjnldsv skjnldsv deleted the feature/implement-vue-virtual-grid branch October 12, 2020 16:34
@Mikescops Mikescops self-assigned this Oct 13, 2020
@TKFRvisionOfficial
Copy link

Hey guys, that is maybe a long shot but have you thought about implementing a little side timeline (a litle line with points on it on the site) where every point is a month so you can easily find pictures. Like at amazon cloud?

@Mikescops
Copy link
Member Author

See #426

@axellebot
Copy link

Should we update the screenshot in Readme.md with the one provided here ?

@sanchosk
Copy link

Would it be somehow possible to separate not only by months, but also by days?

@axellebot
Copy link

Would it be somehow possible to separate not only by months, but also by days?

You should open a new issue to request it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish feature: timeline Related to the timeline section
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Date separators
6 participants