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] Add admin space sidebar details #8192

Merged
merged 20 commits into from
Jan 12, 2023

Conversation

lookacat
Copy link
Contributor

@lookacat lookacat commented Jan 9, 2023

Description

See #8219

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

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

@update-docs
Copy link

update-docs bot commented Jan 9, 2023

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.

@lookacat lookacat force-pushed the admin-spaces-details-edit-sidebar branch 2 times, most recently from b1b3037 to ae29b83 Compare January 11, 2023 13:30
@lookacat lookacat marked this pull request as ready for review January 11, 2023 13:32
Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

I find the panels a bit confusing at the moment. We have SpaceDetails if one space is selected and we have DetailsPanel if no or >1 spaces are selected. I would go for one of the following:

  • Use one panel (SpaceDetails) to handle all cases. Problem with this approach is that the SpaceDetails panel is used in the files-app as well, where we don't want this. Hence I'd rather do it like it's done in the files-app already:
  • Have 3 separate panels for all 3 cases: NoSelection (0 spaces), SpaceDetails (1 space), SpaceDetailsMultiple (2 or more spaces). Those components can live in web-pkg because we might re-use them in the files app as well. The check which panels needs to be displayed would then be done in packages/web-app-admin-settings/src/views/Spaces.vue via the enabled prop of each panel.

And we also need to make sure to not display the space image in SpaceDetails in the admin-settings. You could just introduce another property showImage which defaults to true.

packages/web-app-admin-settings/src/views/Spaces.vue Outdated Show resolved Hide resolved
@lookacat lookacat changed the title WIP Add admin space sidebar edit / details [full-ci] Add admin space sidebar edit / details Jan 12, 2023
@lookacat lookacat force-pushed the admin-spaces-details-edit-sidebar branch from 9f8db69 to 578c06a Compare January 12, 2023 09:15
@lookacat lookacat changed the title [full-ci] Add admin space sidebar edit / details [full-ci] Add admin space sidebar details Jan 12, 2023
@lookacat lookacat requested a review from JammingBen January 12, 2023 09:23
Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

Nice 👍 Just a few minor things I've noticed.

packages/web-app-admin-settings/src/views/Spaces.vue Outdated Show resolved Hide resolved
packages/web-app-admin-settings/src/views/Spaces.vue Outdated Show resolved Hide resolved
@lookacat lookacat requested a review from JammingBen January 12, 2023 09:40
@lookacat lookacat force-pushed the admin-spaces-details-edit-sidebar branch 2 times, most recently from af29e08 to 4e82032 Compare January 12, 2023 10:30
pnpm-lock.yaml Outdated Show resolved Hide resolved
packages/web-pkg/src/constants.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,19 @@
<template>
Copy link
Member

Choose a reason for hiding this comment

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

I think the Details folder should have another parent folder named Spaces. And then please move SpaceNoSelection.vue one level up. Result should be this layout:

- Spaces
  - Details
    - SpaceDetails.vue
    - SpaceDetailsMultiple.vue
  - SpaceNoSelection.vue

In a followup PR you could make use of the SpaceNoSelection component in the Projects.vue view of the files app.

@lookacat lookacat force-pushed the admin-spaces-details-edit-sidebar branch from 8d75fc6 to f668b2d Compare January 12, 2023 11:10
packages/web-app-admin-settings/src/views/Spaces.vue Outdated Show resolved Hide resolved
packages/web-app-admin-settings/src/views/Spaces.vue Outdated Show resolved Hide resolved
<div class="oc-mt-xl">
<div class="oc-flex space-info">
<oc-icon name="layout-grid" size="xxlarge" />
<p v-translate>Multiple spaces selected</p>
Copy link
Member

Choose a reason for hiding this comment

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

Please open a followup issue, the multi selected space panel could show some more info. E.g. how many spaces are selected and what their combined quota values are (total, remaining, used).

@lookacat
Copy link
Contributor Author

Followup: #8222

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.

https://github.com/owncloud/web/pull/8192/files#r1067973359 is still unresolved. Other than that good now. Please also add tests in a followup.

@sonarcloud
Copy link

sonarcloud bot commented Jan 12, 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

69.5% 69.5% Coverage
0.4% 0.4% Duplication

@lookacat lookacat merged commit c72772d into master Jan 12, 2023
@delete-merged-branch delete-merged-branch bot deleted the admin-spaces-details-edit-sidebar branch January 12, 2023 12:42
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.

[Web] Right sidebar for single Space
3 participants