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

Add viewMode toggle for condensed resource table #7976

Merged

Conversation

pascalwengerter
Copy link
Contributor

@pascalwengerter pascalwengerter commented Nov 15, 2022

Description

Adds route-query based toggle between "normal" and "condensed" resource table

Related Issue

Screenshots (if appropriate):

Screenshot 2022-11-15 at 22 34 49

Types of changes

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

Open tasks:

@pascalwengerter pascalwengerter force-pushed the feature/condensed-files-view branch 2 times, most recently from 6f21e60 to b0e257c Compare November 15, 2022 21:44
@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? nevermind, wanted to post this in #7991

@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? nevermind, wanted to post this in #7991

Can't hurt though 😉

@kulmann
Copy link
Member

kulmann commented Nov 22, 2022

Just checked it out, my impression is that the font size is already too small. But it might just be that I'm not "scientific enough" for the scientific mode. 😁

@pascalwengerter
Copy link
Contributor Author

Just checked it out, my impression is that the font size is already too small. But it might just be that I'm not "scientific enough" for the scientific mode. 😁

What does the scientific corner say? @elizavetaRa 🤓

@tbsbdr
Copy link
Contributor

tbsbdr commented Nov 23, 2022

decrease whitespace

Let's only decrease whitespace, to achieve the goal of showing more lines in the viewport.

Example how this looks in dropbox:

screenshot_000011

screenshot_000010

let's not decrease fonstsize, because:

  • this should be done ex. via browser (ctl+ "-") or in the future in the ocis user settings
  • only decreasing fontsize in the fileslist and not in the other ui elements messes up the overal design impression

@tbsbdr
Copy link
Contributor

tbsbdr commented Nov 23, 2022

@pascalwengerter created a custom menu-line-condensed.svg

menu-line-condensed.svg

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24">
<path d="M3 5H21V7H3V5ZM3 9H21V11H3V9ZM3 17H21V19H3V17Z" />
<path d="M21 13H3V15H21V13Z" />
</svg>

Preview:
screenshot_000012

@pascalwengerter pascalwengerter marked this pull request as ready for review November 28, 2022 23:02
@pascalwengerter
Copy link
Contributor Author

@tbsbdr @kulmann please re-review

@@ -0,0 +1,5 @@
Enhancement: Add condensed menu icon
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kulmann not sure if the changelog in the design system still gets updated individually or if there's only one in the repo root from now on?

Copy link
Member

Choose a reason for hiding this comment

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

We need to maintain a separate changelog, because we want to be able to release independently with non aligned version numbers

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, that's what I assumed (and hence the changelog entry^^)

@elizavetaRa
Copy link
Member

elizavetaRa commented Nov 29, 2022

Nice feature, indeed wished by our users. Looking forward to it!

export abstract class ViewModeConstants {
static readonly viewModeDefault: string = 'resource-table'
static readonly viewModeQueryName: string = 'view-mode'
}
Copy link
Member

Choose a reason for hiding this comment

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

Not a huge fan of this use class as enum pattern, I'd rather use typescript types for this nowadays

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've followed what I found in another composable basically, idk if refactoring my/the other solution independently is within the scope of this PR?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with both of you 😆 Would like to get rid of it, but also fine in a separate PR.

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.

UX feels good! 🥳

Could you please add unit tests for the view mode selection, as implemented in ViewOptions.vue? Added some comments.

@@ -59,6 +61,10 @@ export const useResourcesViewDefaults = <T, TT, TU extends any[]>(
fields
})

const currentViewMode = useRouteQuery('view-mode', 'resource-table')
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
const currentViewMode = useRouteQuery('view-mode', 'resource-table')
const currentViewModeQuery = useRouteQuery('view-mode', 'resource-table')

@@ -59,6 +61,10 @@ export const useResourcesViewDefaults = <T, TT, TU extends any[]>(
fields
})

const currentViewMode = useRouteQuery('view-mode', 'resource-table')
const someViewMode = computed((): string => String(currentViewMode.value))
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
const someViewMode = computed((): string => String(currentViewMode.value))
const currentViewMode = computed((): string => queryItemAsString(currentViewMode.value))
  • the import of queryItemAsString from web-pkg/src/composables

<div class="oc-flex oc-flex-middle">
<div class="oc-button-group oc-visible@s oc-mr-s">
<oc-button
:appearance="viewModeCurrent === 'resource-table-condensed' ? 'filled' : 'outline'"
Copy link
Member

Choose a reason for hiding this comment

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

Please use your constants. ;-) You can just disclose them to the template by including them in the object returned by the setup function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, good point. Took me way longer than necessary because I was wondering what const you were talking about exactly^^. Please take a look at e14e464 and let me know which of the two constant variants you prefer! (as in ViewModeConstants.viewModeDefault vis ViewModeConstants.default, e.g.)

:class="hoverableQuickActions && 'hoverable-quick-actions'"
:class="[
hoverableQuickActions && 'hoverable-quick-actions',
{ condensed: viewMode === 'resource-table-condensed' }
Copy link
Member

Choose a reason for hiding this comment

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

Use the const please

@@ -318,6 +321,10 @@ export default defineComponent({
required: false,
default: false
},
viewMode: {
type: String,
default: 'resource-table'
Copy link
Member

Choose a reason for hiding this comment

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

Use the const please

export abstract class ViewModeConstants {
static readonly viewModeDefault: string = 'resource-table'
static readonly viewModeQueryName: string = 'view-mode'
}
Copy link
Member

Choose a reason for hiding this comment

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

I agree with both of you 😆 Would like to get rid of it, but also fine in a separate PR.

name: ViewModeConstants.viewModeQueryName,
defaultValue: ViewModeConstants.viewModeDefault
})
return computed(() => String(unref(perPageQuery)))
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
return computed(() => String(unref(perPageQuery)))
return computed(() => queryItemAsString(unref(perPageQuery)))
  • import from web-pkg/src/composables

@pascalwengerter
Copy link
Contributor Author

UX feels good! 🥳

Could you please add unit tests for the view mode selection, as implemented in ViewOptions.vue? Added some comments.

Thanks for the review, I've addressed all points AFAIK and added a unit test for the button group, basically checking whether the correct button gets shown as outlined/filled after clicking the respective option - is that sufficient for now?

@pascalwengerter
Copy link
Contributor Author

@kulmann could I get a re-review? :)

@kulmann
Copy link
Member

kulmann commented Dec 8, 2022

@kulmann could I get a re-review? :)

Yes, but I'm still waiting for internal UX feedback from @tbsbdr 🙈

@tbsbdr
Copy link
Contributor

tbsbdr commented Dec 8, 2022

awesome! (was blocked as I did not manage to start it locally. Benedikt helped me)
I would have used the menu-line icon for regular-view mode and the condensed icon for condensed-view:

Would love to see it like this

screenshot_000053

menu-line icon for regular-view mode

screenshot_000054

@pascalwengerter
Copy link
Contributor Author

awesome! (was blocked as I did not manage to start it locally. Benedikt helped me) I would have used the menu-line icon for regular-view mode and the condensed icon for condensed-view:

Would love to see it like this

screenshot_000053

menu-line icon for regular-view mode

screenshot_000054

Done!

@sonarcloud
Copy link

sonarcloud bot commented Dec 8, 2022

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 0 Code Smells

77.1% 77.1% Coverage
0.0% 0.0% Duplication

@kulmann
Copy link
Member

kulmann commented Dec 9, 2022

@tbsbdr found a kind of UX flaw: the views Favorites (oc10 only at the moment), Deleted files, Shared with others, Shared via link and Search results have a line below the folder name for displaying the parent folder name/link. For those views the condensed and regular list view mode are identical. I'm torn between (a) not showing the condensed view mode button at all on those pages, or (b) showing the view mode button, knowing that it will not have any effect in the default theme. What do you think @pascalwengerter ?

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.

UX feels good, code is also good now. One hint about tests (ok for later, see comment). My feeling is that we can handle my previous comment in a followup. Let's merge this as first iteration of the condensed view mode.

@@ -95,4 +97,46 @@ describe('ViewOptions', () => {

expect(mockedStore.modules.Files.mutations.SET_HIDDEN_FILES_VISIBILITY).toHaveBeenCalled()
})

it('initially shows normal resource-table by default', () => {
const wrapper = shallowMount(ViewOptions, {
Copy link
Member

Choose a reason for hiding this comment

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

for future tests (not blocking this PR): we introduced a new way of handling mocks in unit tests. See for example

The functions creating the wrappers now return an object, containing mocks, storeOptions and the wrapper. That gives you the chance to manipulate mocks within the test code.

@kulmann kulmann merged commit e013d4b into owncloud:master Dec 9, 2022
@pascalwengerter pascalwengerter deleted the feature/condensed-files-view branch December 21, 2022 15:26
@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.

5 participants