Make viewoption modes configurable via appBar || Extend OcResourceIcon for spaces #8105

pascalwengerter · 2022-12-12T22:39:22Z

Description

Follow-up to #7976

Types of changes

New feature (non-breaking change which adds functionality)

@kulmann @tbsbdr do you want the condensed option on the favorites view also?

update-docs · 2022-12-12T22:39:26Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

kulmann · 2022-12-13T10:24:56Z

packages/web-app-files/src/components/AppBar/AppBar.vue

@@ -28,7 +28,7 @@
        </oc-breadcrumb>
        <shares-navigation v-if="hasSharesNavigation" />
        <div v-if="hasViewOptions || hasSidebarToggle" class="oc-flex">
-          <view-options v-if="hasViewOptions" />
+          <view-options v-if="hasViewOptions" :additional-view-modes="viewModeOptions" />


I'd prefer view-modes as prop name and list all available view modes with it. Otherwise the next dev/reader needs to understand first what additional means.

packages/design-system/src/components/OcResourceIcon/OcResourceIcon.vue

packages/web-app-files/tests/unit/components/AppBar/ViewOptions.spec.js

pascalwengerter · 2022-12-14T19:57:40Z

@kulmann tests are green, ready for re-review :)

JammingBen

I really like that we have a space resource icon now! Should we maybe use the same color as in the sidebar and the projects view? A brighter blue is being used there.

JammingBen · 2022-12-15T09:23:09Z

packages/design-system/src/components/OcResourceIcon/OcResourceIcon.vue

      }
    }
  },
  computed: {
    iconName() {
+      if (this.isSpace) return defaultSpaceIcon


I know it was like this already, but we decided to not use one-line-if-blocks in the project because they are somewhat ugly to read.

.... and because they make debugging more cumbersome and make diffs harder to read when changed later ;-)

So just plain old

if (true) { return "something" }

then, just for clarification?

And what about return "something" ? condition : "fallback", are those types of statements allowed (since I see them used in the same component also)?

I really like that we have a space resource icon now! Should we maybe use the same color as in the sidebar and the projects view? A brighter blue is being used there.

Great eye! I've updated it and would need from @tbsbdr if a ResourceIcon of type "space" should have a more distinct color down the road - for now it'll be the default "passive swatch" color, as it already is in other places right now

return "something" ? condition : "fallback" or const someVariable = "something" ? condition : "fallback" is fine. We just don't want to have the "one-line-return/continue".

packages/design-system/src/components/OcResourceIcon/OcResourceIcon.vue

dschmidt

Hrm now the additional-view-modes moved one level up, I'm not sure that any better than before.
The issue is the same: one needs to look up what it gets added to and one cannot remove the defaults, maybe we should just push the whole list from wherever AppBar is used?

Also I find it weird to pass an arbitrary array of strings into the component where just a few hardcoded ones are handled. If we stick to the hardcoded list of options, I would argue we should instead just use boolean props to enable/disable them.

If we want to pass in a list to make it more flexible, we should pass in an array of mode switch objects, which provide their "query name"(aria), labels, icons, and which we can loop over.

edit:
Currently you're using this list just to check whether it's longer than 1 element or not... that could be a single boolean ;-)

Depending on how flexible you want to make it (and how much effort you want to put in), I'd prefer the "pass an array of objects and render the buttons dynamically" approach, but a single bool param to enable/disable the switcher or individual bool props to disable the currently hardcoded mode buttons would be fine for me as well.

kulmann · 2022-12-15T11:54:44Z

For the future use case of having view modes registered through the extension system it would indeed be beneficial to define a ViewMode interface and pass a list of view mode objects (with icon, label, some id/name prop) through and don't have any hardcoded values in the ViewOptions.vue component.

dschmidt

Love where this is going 🤩

dschmidt · 2022-12-15T22:12:45Z

packages/design-system/src/components/OcResourceIcon/OcResourceIcon.spec.ts

@@ -0,0 +1,26 @@
+import { shallowMount } from '@vue/test-utils'


Please import from web-test-helpers

Still valid / unresolved.

dschmidt · 2022-12-15T22:14:31Z

packages/web-app-files/src/components/AppBar/AppBar.vue

+      if (!this.displayViewModeSwitch) {
+        return []
+      }
+      return [ViewModeConstants.condensedTable, ViewModeConstants.default]
    }


dschmidt · 2022-12-15T22:19:46Z

packages/web-app-files/tests/unit/components/AppBar/ViewOptions.spec.ts

-function getWrapper({ perPage = '100' } = {}) {
+function getWrapper(
+  { perPage = '100' } = {},
+  props: { [key: string]: any } = {


No need for this type

VSCode wanted one pretty badly, but I'll try without...

dschmidt · 2022-12-15T22:21:40Z

packages/web-app-files/src/composables/viewMode/useViewMode.ts

@@ -9,7 +9,7 @@ export function useViewMode<T>(options: ComputedRef<string>): ComputedRef<string

  const viewModeQuery = useRouteQueryPersisted({
    name: ViewModeConstants.queryName,
-    defaultValue: ViewModeConstants.default
+    defaultValue: ViewModeConstants.default.name


I've seen this code a bunch of times, maybe worth putting into its own composable

dschmidt · 2022-12-15T22:22:41Z

packages/web-app-files/src/components/AppBar/ViewOptions.vue

+        :key="viewMode.name"
+        v-oc-tooltip="viewMode.label"
+        :appearance="viewModeCurrent === viewMode.name ? 'filled' : 'outline'"
+        :label="viewMode.label"


I think you need to pass this to $gettext again to make translations actually work

packages/web-app-files/tests/unit/components/AppBar/ViewOptions.spec.ts

kulmann

Found some more nitpick-material, sorry 😅

kulmann · 2022-12-19T20:00:58Z

packages/design-system/src/components/OcResourceIcon/OcResourceIcon.spec.ts

@@ -0,0 +1,26 @@
+import { shallowMount } from '@vue/test-utils'


Still valid / unresolved.

kulmann · 2022-12-19T20:01:36Z

packages/design-system/src/components/OcResourceIcon/OcResourceIcon.spec.ts

+
+const resources = ['file', 'folder', 'space']
+
+function getWrapper(props) {


Please move the getWrapper function below the tests (for consistency with other tests in web).

kulmann · 2022-12-19T20:03:41Z

packages/design-system/src/components/OcResourceIcon/OcResourceIcon.spec.ts

+const resources = ['file', 'folder', 'space']
+
+function getWrapper(props) {
+  return shallowMount(OcResourceIcon, {


Please return an object with key wrapper and your shallowMount call as value, i.e.

return { wrapper: shallowMount(...

for consistency with other tests in web.

kulmann · 2022-12-19T20:10:01Z

packages/web-app-files/src/views/Favorites.vue

@@ -1,7 +1,7 @@
 <template>
  <div class="oc-flex">
    <files-view-wrapper>
-      <app-bar :side-bar-open="sideBarOpen" />
+      <app-bar :display-view-mode-switch="true" :side-bar-open="sideBarOpen" />


Side note: This already is an example where we don't want the condensed view mode (or at least it doesn't make too much sense, since it is not different from the default list view mode), but will want to have the tiles view mode available in the future. :-/ Let's keep it like you have it here for the time being.

kulmann · 2022-12-19T20:12:39Z

packages/web-app-files/src/components/FilesList/ResourceTable.vue

@@ -326,7 +326,7 @@ export default defineComponent({
    },
    viewMode: {
      type: String,
-      default: ViewModeConstants.default
+      default: ViewModeConstants.default.name


You have already introduced the defaultModeName const for this

Apparently slipped through, sry about that

kulmann · 2022-12-19T20:22:26Z

packages/web-app-files/tests/unit/components/AppBar/ViewOptions.spec.ts

+      }
+    )
+    const viewModeSwitchButtons = wrapper.find('[data-testid="viewmode-switch-buttons"]')
+    expect(viewModeSwitchButtons).toMatchSnapshot()


Would like to avoid a snapshot test for this. Should already be sufficient to check the presence of the element.
If you ever find a situation where a snapshot test is useful, please do so on the .html() result of the element. Will be required shortly.

I'm somewhat torn about this - I know the oC web team in general is not a huge fan of snapshot tests, however in this case it's not only asserting that there is an element with a viewmode-switch-buttons data-testid, but also that there's two buttons, each with an icon inside (and checks for the momentarily correct CSS classes etc)

Also fine to keep it... I'd go the way of asserting that there are x buttons and ignoring the icon assertion for example.

But: please change line 50 to expect(viewModeSwitchButtons.html()).toMatchSnapshot()

packages/design-system/src/components/OcResourceIcon/OcResourceIcon.vue

kulmann · 2022-12-19T20:26:41Z

packages/design-system/src/components/OcResourceIcon/OcResourceIcon.vue

 import OcIcon from '../OcIcon/OcIcon.vue'
-import iconNameMap from '../../helpers/resourceIconExtensionMapping'
-import iconColorMap from '../../helpers/resourceIconColorExtensionMapping'
+import { AVAILABLE_SIZES } from '../../helpers/constants'


Suggested change

import { AVAILABLE_SIZES } from '../../helpers/constants'

import { AVAILABLE_SIZES } from '../../helpers'

(at least that's what my IDE is complaining about ;-) )

It's possible but can eventually lead to circular import (stumbled upon this last week), will fix for now but keep in mind pls :)

…nd extract IconFillType to designsystem

…Space+Favorties view

kulmann

🤘

sonarcloud · 2022-12-19T21:47:37Z

SonarCloud Quality Gate failed.

2 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

75.0% Coverage
0.0% Duplication

Author: Pascal Wengerter <pascal@wengerter.info> Date: Tue Dec 20 00:14:26 2022 +0100 Make viewoption modes configurable via appBar || Extend OcResourceIcon for spaces (#8105)

pascalwengerter changed the title ~~WIP: Feature/add appbar viewoptions modes~~ WIP: Make viewoption modes configurable via appBar Dec 12, 2022

pascalwengerter force-pushed the feature/add-appbar-viewoptions-modes branch from 38da3d6 to e5adb8f Compare December 13, 2022 08:02

kulmann requested changes Dec 13, 2022

View reviewed changes

pascalwengerter force-pushed the feature/add-appbar-viewoptions-modes branch 3 times, most recently from 11a4863 to dfccb41 Compare December 13, 2022 22:35

pascalwengerter marked this pull request as ready for review December 13, 2022 22:35

pascalwengerter requested a review from kulmann December 13, 2022 22:35

pascalwengerter force-pushed the feature/add-appbar-viewoptions-modes branch from dfccb41 to 9971b3f Compare December 13, 2022 22:45

pascalwengerter changed the title ~~WIP: Make viewoption modes configurable via appBar~~ Make viewoption modes configurable via appBar || Extend OcResourceIcon for spaces Dec 13, 2022

dschmidt reviewed Dec 13, 2022

View reviewed changes

packages/design-system/src/components/OcResourceIcon/OcResourceIcon.vue Outdated Show resolved Hide resolved

packages/web-app-files/tests/unit/components/AppBar/ViewOptions.spec.js Outdated Show resolved Hide resolved

pascalwengerter requested review from dschmidt and kulmann and removed request for kulmann and dschmidt December 14, 2022 19:18

JammingBen reviewed Dec 15, 2022

View reviewed changes

dschmidt requested changes Dec 15, 2022

View reviewed changes

pascalwengerter force-pushed the feature/add-appbar-viewoptions-modes branch from 36d901e to 194a8fc Compare December 15, 2022 21:46

dschmidt reviewed Dec 15, 2022

View reviewed changes

pascalwengerter requested review from dschmidt, JammingBen and kulmann and removed request for kulmann, dschmidt and JammingBen December 16, 2022 08:33

pascalwengerter force-pushed the feature/add-appbar-viewoptions-modes branch 3 times, most recently from 5a581fa to 584cf7b Compare December 16, 2022 19:16

dschmidt reviewed Dec 16, 2022

View reviewed changes

packages/web-app-files/tests/unit/components/AppBar/ViewOptions.spec.ts Show resolved Hide resolved

dschmidt approved these changes Dec 16, 2022

View reviewed changes

kulmann requested changes Dec 19, 2022

View reviewed changes

pascalwengerter added 4 commits December 19, 2022 22:05

Minor code cleanup

052fdf7

Make OcResourceIcon accept space resource, refactor size validation a…

4e448f6

…nd extract IconFillType to designsystem

Make condensed viewMode configurable in appBar && limit it to Generic…

546dd48

…Space+Favorties view

Address code review

4046cbe

pascalwengerter force-pushed the feature/add-appbar-viewoptions-modes branch from 584cf7b to adbe049 Compare December 19, 2022 21:18

Sort design-system dependencies, add web-client as peerDep

c69ac0c

pascalwengerter force-pushed the feature/add-appbar-viewoptions-modes branch from adbe049 to 33cdf83 Compare December 19, 2022 21:40

kulmann approved these changes Dec 19, 2022

View reviewed changes

Address code review II

33cdf83

kulmann merged commit 508c716 into owncloud:master Dec 19, 2022

ownclouders pushed a commit that referenced this pull request Dec 19, 2022

commit 508c716

1b65075

Author: Pascal Wengerter <pascal@wengerter.info> Date: Tue Dec 20 00:14:26 2022 +0100 Make viewoption modes configurable via appBar || Extend OcResourceIcon for spaces (#8105)

pascalwengerter deleted the feature/add-appbar-viewoptions-modes branch December 21, 2022 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make viewoption modes configurable via appBar || Extend OcResourceIcon for spaces #8105

Make viewoption modes configurable via appBar || Extend OcResourceIcon for spaces #8105

pascalwengerter commented Dec 12, 2022 •

edited

Loading

update-docs bot commented Dec 12, 2022

kulmann Dec 13, 2022

pascalwengerter commented Dec 14, 2022

JammingBen left a comment

JammingBen Dec 15, 2022

dschmidt Dec 15, 2022

pascalwengerter Dec 15, 2022

pascalwengerter Dec 15, 2022

JammingBen Dec 16, 2022

dschmidt left a comment •

edited

Loading

kulmann commented Dec 15, 2022

dschmidt left a comment •

edited

Loading

dschmidt Dec 15, 2022

kulmann Dec 19, 2022

dschmidt Dec 15, 2022

dschmidt Dec 15, 2022

pascalwengerter Dec 15, 2022

dschmidt Dec 15, 2022

dschmidt Dec 15, 2022

kulmann left a comment

kulmann Dec 19, 2022

kulmann Dec 19, 2022

kulmann Dec 19, 2022

kulmann Dec 19, 2022

pascalwengerter Dec 19, 2022

kulmann Dec 19, 2022

pascalwengerter Dec 19, 2022

kulmann Dec 19, 2022

pascalwengerter Dec 19, 2022

kulmann Dec 19, 2022

kulmann Dec 19, 2022

pascalwengerter Dec 19, 2022

kulmann left a comment

sonarcloud bot commented Dec 19, 2022

		@@ -0,0 +1,26 @@
		import { shallowMount } from '@vue/test-utils'


		const resources = ['file', 'folder', 'space']

		function getWrapper(props) {

	import { AVAILABLE_SIZES } from '../../helpers/constants'
	import { AVAILABLE_SIZES } from '../../helpers'

Make viewoption modes configurable via appBar || Extend OcResourceIcon for spaces #8105

Make viewoption modes configurable via appBar || Extend OcResourceIcon for spaces #8105

Conversation

pascalwengerter commented Dec 12, 2022 • edited Loading

Description

Types of changes

update-docs bot commented Dec 12, 2022

Choose a reason for hiding this comment

pascalwengerter commented Dec 14, 2022

JammingBen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschmidt left a comment • edited Loading

Choose a reason for hiding this comment

kulmann commented Dec 15, 2022

dschmidt left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kulmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kulmann left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Dec 19, 2022

pascalwengerter commented Dec 12, 2022 •

edited

Loading

dschmidt left a comment •

edited

Loading

dschmidt left a comment •

edited

Loading