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

Bugfix: Gridview whitespace autosize #10118

Merged
merged 28 commits into from
Dec 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
dd91ac5
Bugfix: Gridview whitespace autosize
lookacat Dec 5, 2023
def728c
Fix large scale
lookacat Dec 5, 2023
b74e34f
Fix viewoption sizes
lookacat Dec 5, 2023
f338af7
Add changelog
lookacat Dec 5, 2023
ca58433
Update unittests
lookacat Dec 5, 2023
c8be4ef
WIP
lookacat Dec 8, 2023
2a1934c
Solution?
lookacat Dec 11, 2023
bb517fd
WIP
lookacat Dec 12, 2023
2f56114
WIP
lookacat Dec 12, 2023
c12a7a8
Remove dev leftover
lookacat Dec 12, 2023
72c1720
cleanup code
lookacat Dec 12, 2023
7cc5d54
Calculate max tiles count per row / still needs to be recalculated wh…
lookacat Dec 15, 2023
feeb5a2
Implement watch for tile-size slider changes
lookacat Dec 15, 2023
bbca187
Rename rowCount
lookacat Dec 15, 2023
e5962ea
Lint, change var to const, remove unnecessary computed
AlexAndBear Dec 16, 2023
5bd1806
Hide from screen readers
AlexAndBear Dec 16, 2023
1edf3d4
feat: introduce useTileSize composable and watch that for tile sizes
kulmann Dec 18, 2023
7c652f8
Fix undefined issues
lookacat Dec 18, 2023
18b8a9d
slim mount function
lookacat Dec 18, 2023
5bdb247
fix: tile pixels and ghost tile count
kulmann Dec 18, 2023
a9bbe6c
test: fix viewOptions unit tests
kulmann Dec 18, 2023
06f1ad4
Fixing ResourceTiles.spec.ts tests (WIP)
lookacat Dec 18, 2023
ed1870f
Fix ResourceTiles.spec.ts
lookacat Dec 19, 2023
63eafdd
Remove dev leftover
lookacat Dec 19, 2023
ae3d2b2
Update tileSizePixels in tests
lookacat Dec 19, 2023
3b59e97
docs: add tile size vars to theme docs
kulmann Dec 19, 2023
a00230c
Address PR issues
lookacat Dec 20, 2023
ed4c794
Make linter happy
lookacat Dec 20, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions changelog/unreleased/bugfix-tiles-view-whitespace
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
Bugfix: Tilesview has whitespace

We've fixed a bug that caused the tiles-view to have whitespace
on the right side of the screen which is not optimal for efficiant
space management.

https://github.com/owncloud/web/pull/10118
https://github.com/owncloud/web/issues/10040
8 changes: 6 additions & 2 deletions docs/theming/_index.md
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,9 @@ Size variables get prepended with `--oc-size-`, so e.g. _"icon-default"_ creates
"icon-default": "",
"max-height-logo": "",
"max-width-logo": "",
"width-medium": ""
"width-medium": "",
"tiles-default": "",
"tiles-resize-step": ""
}
}
```
Expand Down Expand Up @@ -426,7 +428,9 @@ A full template for your custom theme is provided below, and you can use the ins
"icon-default": "",
"max-height-logo": "",
"max-width-logo": "",
"width-medium": ""
"width-medium": "",
"tiles-default": "",
"tiles-resize-step": ""
},
"spacing": {
"xsmall": "",
Expand Down
4 changes: 2 additions & 2 deletions packages/design-system/src/tokens/ods/size.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,6 @@ size:
value: 14px
tiles:
resize-step:
value: 12rem
value: 9rem
default:
value: 14rem
value: 9rem
130 changes: 98 additions & 32 deletions packages/web-app-files/src/components/FilesList/ResourceTiles.vue
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
</oc-list>
</oc-drop>
</div>
<oc-list class="oc-tiles oc-flex" :class="resizable ? 'resizableTiles' : ''">
<oc-list class="oc-tiles oc-flex">
<li v-for="resource in data" :key="resource.id" class="oc-tiles-item has-item-context-menu">
<oc-tile
:ref="(el) => (tileRefs.tiles[resource.id] = el)"
Expand Down Expand Up @@ -85,6 +85,16 @@
</template>
</oc-tile>
</li>
<li
v-for="index in ghostTilesCount"
:key="`ghost-tile-${index}`"
class="ghost-tile"
:aria-hidden="true"
>
<div>
{{ viewSize }}
</div>
</li>
</oc-list>
<Teleport v-if="dragItem" to="body">
<oc-ghost-element ref="ghostElementRef" :preview-items="[dragItem, ...dragSelection]" />
Expand All @@ -96,21 +106,37 @@
</template>

<script lang="ts">
import { onBeforeUpdate, defineComponent, nextTick, PropType, computed, ref, unref } from 'vue'
import {
computed,
defineComponent,
nextTick,
onBeforeUnmount,
onBeforeUpdate,
onMounted,
PropType,
ref,
unref,
watch
} from 'vue'
import { useGettext } from 'vue3-gettext'
import { Resource, SpaceResource } from '@ownclouders/web-client'
import { useStore, SortDir, SortField, ViewModeConstants } from '@ownclouders/web-pkg'
import { ImageDimension } from '@ownclouders/web-pkg'
import { createFileRouteOptions } from '@ownclouders/web-pkg'
import { displayPositionedDropdown } from '@ownclouders/web-pkg'
import { createLocationSpaces } from '@ownclouders/web-pkg'
import { ContextMenuQuickAction } from '@ownclouders/web-pkg'

// Constants should match what is being used in OcTable/ResourceTable
// Alignment regarding naming would be an API-breaking change and can
// Be done at a later point in time?
import { useResourceRouteResolver } from '@ownclouders/web-pkg'
import { eventBus } from '@ownclouders/web-pkg'
import {
ContextMenuQuickAction,
createFileRouteOptions,
createLocationSpaces,
displayPositionedDropdown,
eventBus,
ImageDimension,
SortDir,
SortField,
useResourceRouteResolver,
useStore,
useTileSize,
ViewModeConstants
} from '@ownclouders/web-pkg'

export default defineComponent({
name: 'ResourceTiles',
Expand All @@ -123,10 +149,6 @@ export default defineComponent({
type: Array as PropType<Resource[]>,
default: () => []
},
resizable: {
type: Boolean,
default: false
},
selectedIds: {
type: Array,
default: () => []
Expand Down Expand Up @@ -179,6 +201,7 @@ export default defineComponent({

const dragItem = ref()
const ghostElementRef = ref()

const tileRefs = ref({
tiles: [],
dropBtns: []
Expand Down Expand Up @@ -308,7 +331,6 @@ export default defineComponent({
}
return sizeMap[props.viewSize] ?? 'xlarge'
})

onBeforeUpdate(() => {
tileRefs.value = {
tiles: [],
Expand Down Expand Up @@ -363,6 +385,50 @@ export default defineComponent({
context.emit('fileDropped', resource.id)
}

const viewWidth = ref(0)
const updateViewWidth = () => {
const element = document.getElementById('tiles-view')
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels quite weird to set the id tiles-view from the outside and then reference it here because it creates an indirect dependency. I think it would be cleaner to set the id here in the component, or better use a template ref.

const style = getComputedStyle(element)
const paddingLeft = parseInt(style.getPropertyValue('padding-left'), 10) | 0
const paddingRight = parseInt(style.getPropertyValue('padding-right'), 10) | 0
viewWidth.value = element.clientWidth - paddingLeft - paddingRight
}
const { tileSizePixels: tileSizePixelsBase } = useTileSize()
const gapSizePixels = computed(() => {
return parseFloat(getComputedStyle(document.documentElement).fontSize)
})
const maxTiles = computed(() => {
return unref(tileSizePixelsBase)
? Math.floor(unref(viewWidth) / (unref(tileSizePixelsBase) + unref(gapSizePixels)))
: 0
})
const ghostTilesCount = computed(() => {
const remainder = unref(maxTiles) ? props.data.length % unref(maxTiles) : 0
if (!remainder) {
return 0
}
return unref(maxTiles) - remainder
})

const tileSizePixels = computed(() => {
return unref(viewWidth) / unref(maxTiles) - unref(gapSizePixels)
})
watch(
tileSizePixels,
(px: number) => {
document.documentElement.style.setProperty(`--oc-size-tiles-actual`, `${px}px`)
},
{ immediate: true }
)

onMounted(() => {
window.addEventListener('resize', updateViewWidth)
updateViewWidth()
})
onBeforeUnmount(() => {
window.removeEventListener('resize', updateViewWidth)
})

return {
areFileExtensionsShown,
emitTileClick,
Expand All @@ -383,7 +449,8 @@ export default defineComponent({
dragStart,
dragSelection,
fileDropped,
setDropStyling
setDropStyling,
ghostTilesCount
}
},
data() {
Expand All @@ -398,24 +465,10 @@ export default defineComponent({
.oc-tiles {
column-gap: 1rem;
display: grid;
grid-template-columns: repeat(auto-fill, minmax(auto, var(--oc-size-tiles-default)));
grid-template-columns: repeat(auto-fit, minmax(var(--oc-size-tiles-actual), 1fr));
justify-content: flex-start;
row-gap: 1rem;

&.resizableTiles {
grid-template-columns: repeat(auto-fill, minmax(auto, var(--oc-size-tiles-resize-step)));
}

@media only screen and (max-width: 640px) {
grid-template-columns: 80%;
justify-content: center;
padding: var(--oc-space-medium) 0;

&.resizableTiles {
grid-template-columns: 80%;
}
}

&-item-drop-highlight {
background-color: var(--oc-color-input-border) !important;
}
Expand All @@ -441,4 +494,17 @@ export default defineComponent({
}
}
}

.ghost-tile {
display: list-item;

div {
opacity: 0;
box-shadow: none;
height: 100%;
display: flex;
flex-flow: column;
outline: 1px solid var(--oc-color-border);
}
}
</style>
1 change: 0 additions & 1 deletion packages/web-app-files/src/views/spaces/GenericSpace.vue
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@
v-model:selectedIds="selectedResourcesIds"
:data="paginatedResources"
class="oc-px-m oc-pt-l"
:resizable="true"
:target-route-callback="resourceTargetRouteCallback"
:space="space"
:drag-drop="true"
Expand Down
1 change: 0 additions & 1 deletion packages/web-app-files/src/views/spaces/Projects.vue
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
v-model:selectedIds="selectedResourcesIds"
class="oc-px-m"
:data="paginatedItems"
:resizable="true"
:sort-fields="sortFields"
:sort-by="sortBy"
:sort-dir="sortDir"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@ import ResourceTiles from 'web-app-files/src/components/FilesList/ResourceTiles.
import { sortFields } from 'web-app-files/src/helpers/ui/resourceTiles'
import { Resource } from '@ownclouders/web-client'
import { mock } from 'jest-mock-extended'
jest.mock('@ownclouders/web-pkg', () => ({
...jest.requireActual('@ownclouders/web-pkg'),
useTileSize: jest.fn().mockReturnValue({
tileSizePixels: 100
})
}))

const spacesResources = [
{
Expand All @@ -24,8 +30,37 @@ const spacesResources = [
]

describe('ResourceTiles component', () => {
it('renders an array of spaces correctly', () => {
const originalGetElementById = document.getElementById
const originalGetComputedStyle = window.getComputedStyle
beforeEach(() => {
const mockElement = {
clientWidth: 800
}
;(document as any).getElementById = jest.fn((id) => {
if (id === 'tiles-view') {
return mockElement
}
return originalGetElementById.call(document, id)
})
window.getComputedStyle = jest.fn().mockImplementation(() => {
return {
getPropertyValue: (propName) => {
switch (propName) {
case '--oc-size-tiles-default':
return '9rem'
case '--oc-size-tiles-resize-step':
return '9rem'
default:
return originalGetComputedStyle(document.documentElement).getPropertyValue(propName)
}
},
fontSize: '14px'
}
})
})
it('renders an array of spaces correctly', async () => {
const { wrapper } = getWrapper({ data: spacesResources })
await wrapper.vm.$nextTick()
expect(wrapper.html()).toMatchSnapshot()
})

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
exports[`ResourceTiles component renders a footer slot 1`] = `
<div>
<!--v-if-->
<oc-list class="oc-tiles oc-flex"></oc-list>
<oc-list class="oc-tiles oc-flex"> </oc-list>
<!--v-if-->
<div class="oc-tiles-footer">Hello, ResourceTiles footer!</div>
</div>
Expand All @@ -19,6 +19,21 @@ exports[`ResourceTiles component renders an array of spaces correctly 1`] = `
<li class="oc-tiles-item has-item-context-menu">
<oc-tile draggable="false" is-resource-selected="false" resource="[object Object]" resource-icon-size="xlarge" resource-route="[object Object]"></oc-tile>
</li>
<li aria-hidden="true" class="ghost-tile">
<div>1</div>
</li>
<li aria-hidden="true" class="ghost-tile">
<div>1</div>
</li>
<li aria-hidden="true" class="ghost-tile">
<div>1</div>
</li>
<li aria-hidden="true" class="ghost-tile">
<div>1</div>
</li>
<li aria-hidden="true" class="ghost-tile">
<div>1</div>
</li>
</oc-list>
<!--v-if-->
<div class="oc-tiles-footer"></div>
Expand Down
21 changes: 0 additions & 21 deletions packages/web-pkg/src/components/ViewOptions.vue
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@
max="6"
class="oc-range"
data-testid="files-tiles-size-slider"
@input="setTilesViewSize"
/>
</li>
</oc-list>
Expand Down Expand Up @@ -166,15 +165,6 @@ export default defineComponent({
defaultValue: ViewModeConstants.tilesSizeDefault.toString()
})

const setTilesViewSize = () => {
const rootStyle = (document.querySelector(':root') as HTMLElement).style
const currentSize = rootStyle.getPropertyValue('--oc-size-tiles-resize-step')
const newSize = `${(unref(viewSizeQuery) as any) * 12}rem`
if (!currentSize || currentSize !== newSize) {
rootStyle.setProperty(`--oc-size-tiles-resize-step`, newSize)
}
}

const setItemsPerPage = (itemsPerPage: string) => {
return router.replace({
query: {
Expand All @@ -197,24 +187,13 @@ export default defineComponent({
{ immediate: true, deep: true }
)

watch(
viewSizeQuery,
(size) => {
if (size) {
setTilesViewSize()
}
},
{ immediate: true }
)

return {
ViewModeConstants,
viewModeCurrent: viewModeQuery,
viewSizeCurrent: viewSizeQuery,
itemsPerPageCurrent: itemsPerPageQuery,
queryParamsLoading,
queryItemAsString,
setTilesViewSize,
setItemsPerPage,
setViewMode,
viewOptionsButtonLabel: $gettext('Display customization options of the files list')
Expand Down
1 change: 1 addition & 0 deletions packages/web-pkg/src/composables/viewMode/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export * from './constants'
export * from './useTileSize'
export * from './useViewMode'
Loading