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

[full-ci] Display resources in GenericSpace as tiles #7991

Merged
merged 26 commits into from
Feb 1, 2023

Conversation

pascalwengerter
Copy link
Contributor

@pascalwengerter pascalwengerter commented Nov 17, 2022

Related Issue

Screenshots (if appropriate):

Screenshot 2023-01-10 at 15 57 54

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@kulmann
Copy link
Member

kulmann commented Nov 21, 2022

I have merged #7940 today - could you rebase this PR and include all your design system changes directly in web? 😁

@pascalwengerter
Copy link
Contributor Author

I have merged #7940 today - could you rebase this PR and include all your design system changes directly in web? 😁

Done! 🐎

flex-wrap: wrap;
flex-direction: row;
gap: 15px;
justify-content: flex-start;
Copy link
Member

@kulmann kulmann Nov 23, 2022

Choose a reason for hiding this comment

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

Suggested change
justify-content: flex-start;
justify-content: space-around;

is much more pleasant for the eye regarding spacing, but any time the last row is incomplete (i.e. not the same number of tiles as in the other rows) the tiles in the last row are centered, which doesn't look good... maybe we need some magic dummy tiles.

Copy link
Member

@dschmidt dschmidt Nov 23, 2022

Choose a reason for hiding this comment

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

The horizontal spacing is nice, but then the vertical spacing doesn't match with this change alone

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Could you re-render with the updated tile size during modification of the slider value? The URL query param changes immediately when you reach the next tick on the slider, but the tiles only update after you finished sliding the slider.

The main concern I have with your proposal of calculating the tile sizes based on the viewport size is that it stops to work good as soon as the viewport width becomes VERY large. E.g. @dschmidt with his ultra wide screen would have quite large tiles in the smallest scale. That's the reason why we wanted to have tile sizes based on multiples of a certain REM value, not based on the viewport size. I still think that it's a good idea... (edit: that means that I'd root for your "option 1", which is the default in this PR. "option 2" doesn't work for large screens in my opinion)

@dschmidt
Copy link
Member

with rem option in ViewOptions.vue:
Screenshot_20221123_101841

with percent option:
Screenshot_20221123_101449

I agree with @kulmann that the latter is waaay to big for the smallest setting

@tbsbdr
Copy link
Contributor

tbsbdr commented Nov 23, 2022

Refined proposal I discussed with @kulmann

  • Size of tiles: Adjust via slider
  • Number of tiles: depends on viewport size
  • Spacing: use "space-aroundish"-behavior to achieve same effect as in windows explorer or MacOS finder
  • Spacing vertical spacing between resources does not vary in relation to the viewport, but should be in similar range to the horizontal spacing 🪄 vertical spacing increases if tiles size gets increased (c.f. video attached & screenshot or play around with Win explorer / finder tiles view to get the intended experience)

space-aroundish"-behavior

screenshot_000014-converted.mp4

vertical space increases ~ tiles size

screenshot_000016

packages/design-system/src/components/OcTiles/OcTiles.vue Outdated Show resolved Hide resolved
createFileAction,
createFolderLink
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Just a nitpick, we usually just re-rexport from index.ts and put composables in their own files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point thx, I will split it up (and also add tests etc) when it's time ;P

:size="totalFilesSize"
/>
</template>
</resource-tiles>
Copy link
Member

Choose a reason for hiding this comment

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

Is the plan to use component :is='...' in the long run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not 100% sure yet tbh, I faced some problems of the GenericSpace not handling events being thrown by the switched-out component. Up for discussion I suppose, I can see another possible strategy that's pretty lean code-wise evolve :)

@pascalwengerter pascalwengerter changed the title Proof-of-concept: tiles-view Display resources in GenericSpace as tiles Jan 10, 2023
@pascalwengerter
Copy link
Contributor Author

@kulmann could I get a cursory review here (mostly about UI/UX aspects, the code changes will receive some what-goes-where refactoring and are not yet final)?
Not sure what is in scope of the linked ticket

Known issues as of now:

  • Navigation in none-root space has some hickups, seems like the fileId gets added to the route at some point and brakes the router?

@pascalwengerter pascalwengerter marked this pull request as ready for review January 10, 2023 14:59
@pascalwengerter
Copy link
Contributor Author

@kulmann could I get a cursory review here (mostly about UI/UX aspects, the code changes will receive some what-goes-where refactoring and are not yet final)? Not sure what is in scope of the linked ticket

Known issues as of now:

* [ ]  Navigation in none-root space has some hickups, seems like the fileId gets added to the route at some point and brakes the router?

Update - I investigated some more and navigation inside folder in a space does work, but is horribly inefficient (browser freezes, and navigation takes place ~40s after clicking). My suspectis the dynamic ref implementation which is needed for the Contextmenu logic

@pascalwengerter
Copy link
Contributor Author

@kulmann could I get a cursory review here (mostly about UI/UX aspects, the code changes will receive some what-goes-where refactoring and are not yet final)? Not sure what is in scope of the linked ticket
Known issues as of now:

* [ ]  Navigation in none-root space has some hickups, seems like the fileId gets added to the route at some point and brakes the router?

Update - I investigated some more and navigation inside folder in a space does work, but is horribly inefficient (browser freezes, and navigation takes place ~40s after clicking). My suspectis the dynamic ref implementation which is needed for the Contextmenu logic

Update - starting it with pnpm build:w (production build) as pointed out by @dschmidt does the trick, apparently all the deprecation warnings just wrangle the thing down...

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Awesome state / progress! Some tiny nitpicks, see comments. And I have two issues that you might decide to solve in a followup (also see screenshot):

  • text truncation happens too early, there is a lot of horizontal white space left.
  • preview size is sometimes shrinked / stretched. That's weird. object-fit and aspect-ratio don't seem to have an effect, I can disable the props and nothing changes (I wanted to try out if contain looks better, but didn't happen).

Screenshot 2023-01-13 at 16 11 35

background-color: var(--oc-color-background-highlight) !important;
box-shadow: 3px 0 10px rgb(0 0 0 / 15%);
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the box-shadow as none like before, but add outline: 1px solid var(--oc-color-border); instead. Discussed it with @tbsbdr that's how we want to start. Already had too much harsh feedback with shadows in other places of oC Web. 😅

return [
ViewModeConstants.tilesView,
ViewModeConstants.condensedTable,
ViewModeConstants.default
Copy link
Member

Choose a reason for hiding this comment

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

Please change order to

  1. condensed
  2. default
  3. tiles

@click="$emit('click', $event)"
/>
<div class="oc-flex oc-flex-middle oc-text-truncate resource-name-wrapper">
<oc-resource :resource="resource" :folder-link="resourceRoute" @click="$emit('click')" />
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<oc-resource :resource="resource" :folder-link="resourceRoute" @click="$emit('click')" />
<oc-resource :resource="resource" :folder-link="resourceRoute" :is-thumbnail-displayed="false" @click="$emit('click')" />

Thumbnails must be disabled here. Otherwise you might have glitchy situations where the OcTile has a large preview at the top and a small thumbnail in the bottom area. Managed to create that situation, don't have a formula to reproduce it, sorry. 👀

@tbsbdr
Copy link
Contributor

tbsbdr commented Jan 13, 2023

Played around with gap- and tiles sizes. The easiest way to avoid too much unbalanced gaps is to reduce the tiles size.

Tilesize 250px -> big gaps ❌

screenshot_000192.mp4

Tilesize 160px -> small gaps ✅

screenshot_000194.mp4
.oc-tiles {
  display: grid;
  grid-template-columns: repeat(auto-fill, 160px);
  row-gap: 32px;
  column-gap: 8px;
  justify-content: space-between;

  .oc-tiles-item {
    width: 160px;

    @media (max-width: $oc-breakpoint-xsmall-max) {
      width: 100%;
    }
  }
}

I also removed the leading file-icon from the small tiles. @kulmann @pascalwengerter I'm in favour of small tiles as long as we don't have the slider. Do you agree?

@pascalwengerter pascalwengerter force-pushed the feature/tiles-view branch 2 times, most recently from 2779902 to e4f086d Compare January 16, 2023 20:59
@ownclouders
Copy link
Contributor

ownclouders commented Jan 16, 2023

Results for acceptance oC10 https://drone.owncloud.com/owncloud/web/32303/20/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUIPreview-mediaPreview_feature-L138.png

webUIPreview-mediaPreview_feature-L138.png

webUIPreview-mediaPreview_feature-L139.png

webUIPreview-mediaPreview_feature-L139.png

webUIPreview-mediaPreview_feature-L140.png

webUIPreview-mediaPreview_feature-L140.png

webUIPreview-mediaPreview_feature-L77.png

webUIPreview-mediaPreview_feature-L77.png

webUIPreview-mediaPreview_feature-L84.png

webUIPreview-mediaPreview_feature-L84.png

@pascalwengerter
Copy link
Contributor Author

Thanks @JammingBen for a second pair of eyes! @kulmann ready for final review and please squash-merge this!

@sonarcloud
Copy link

sonarcloud bot commented Feb 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

66.5% 66.5% Coverage
0.0% 0.0% Duplication

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

😍

@kulmann kulmann merged commit 4ad8116 into owncloud:master Feb 1, 2023
@pascalwengerter pascalwengerter deleted the feature/tiles-view branch February 5, 2023 08:36
@micbar micbar mentioned this pull request May 3, 2023
89 tasks
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.

Display simple files preview as grid
6 participants