-
Notifications
You must be signed in to change notification settings - Fork 159
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
Divide "Shared with me" view into pending, accepted and declined sections #4989
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for opening this PR! I'll take a closer look at the Vue component later on, some general comments ahead:
- We pull our translations from https://www.transifex.com/, so please revert the changes to
translations.json
. Once your PR is merged, we'll translate all the strings in Transifex and then update them in theweb
repo - Could you open issues in the respective repos (oCIS/reva) for the "open tasks" in your PR and link them here? Then it's clearer who has to do what and if it has been done ;)
@pascalwengerter I linked the corresponding issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thank you for your contribution! Sorry that it took so long to review... Added some remarks. Also: we have a shareStatus
const, which you can use in your code changes to use named variables instead integers for the different share status types. In the script you can use it as shareStatus.pending
, shareStatus.accepted
and shareStatus.declined
. For using it also in the template you need to add shareStatus
as a new line to the data
object (after line 268 at the moment).
<!-- Pending shares: show always --> | ||
<div v-if="filterDataByStatus(activeFiles, 1).length > 0" id="pending-shares"> | ||
<div class="oc-app-bar shares-bar"> | ||
<h4><translate>Pending Shares</translate></h4> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a h1
for the page title (hidden, only for screen readers) and then no other h-tags, so technically (not talking about styling) this should be a h2
, right?
v-if="filterDataByStatus(activeFiles, 0).length === 0 && !getShowDeclined()" | ||
id="show-declined-shares-btn" | ||
@click="setShowDeclined(true)" | ||
>Show declined shares</a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the v-translate
directive to the a
tag
class="uk-text-small oc-ml file-row-share-status-text uk-text-baseline" | ||
v-text="getShareStatusText(resource.status)" | ||
/> | ||
<a class="show-hide-pending" @click="setShowAllPending(true)"> Show all</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you switch this from an a
tag to an oc-button
tag with appearance="raw"
? Reason: this is no navigation, but a modification/action on the current page. Therefor it's supposed to be a button, not a link.
this.$forceUpdate() | ||
}, | ||
|
||
getShowDeclined() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be fine with accessing the data prop directly in the template, without a getter. But up to you
this.$forceUpdate() | ||
}, | ||
|
||
getShowAllPending() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, I'd be fine with accessing the data prop directly in the template, without a getter. But up to you
}, | ||
|
||
filterDataByStatus(data, status) { | ||
return data.filter((item) => item.status === status) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can simplify this to return data.filter((item) => item.status === status)
, because the result of filter
is always an array (also an empty one if there are no matching elements)
}) | ||
response = await response.json() | ||
if (response.ocs.data.length > 0) { | ||
if (response.ocs.data && response.ocs.data.length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use if (response.ocs.data?.length > 0) {
here (optional chaining, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining )
@@ -241,18 +438,48 @@ export default { | |||
this.getToken | |||
) | |||
this.UPDATE_RESOURCE(sharedResource) | |||
this.loadResources() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please drop this line, we'll solve that outside this PR
@elizavetaRa just checked CI and it already fails in the |
💥 Acceptance tests oc10-integration-notifications failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15323/
|
💥 Acceptance tests webUISharingNotifications failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15323/
|
💥 Acceptance tests IntegrationApp1 failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15323/
|
💥 Acceptance tests webUISharingBasic failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15323/
|
@pascalwengerter I set the linting, some acceptance tests are indeed broken |
} | ||
.show-hide-pending { | ||
text-align: center; | ||
padding-bottom: 12px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use pixel values for padding / margin. You could either use our margin and padding classes from the design system on the element where you added the show-hide-pending
class. I.e. remove the padding from the class here and add the respective padding class on the button where you used it. Documentation is available here: https://owncloud.design/#/Utilities/Padding The class that is interesting here for you is oc-pb-s
, which stands for **p**adding **b**ottom **s**mall
.
Another way would be to use the css variable for padding sizes.
This is important so that your theming config regarding spacing will take effect here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can see the default spacing pixel sizes here: https://owncloud.design/#/Design%20Tokens
} | ||
|
||
#pending-highlight { | ||
background-color: aliceblue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you check if there is a css variable from our color design tokens? Ideally we don't to have explicit colors anymore in the web code, so that theming works properly. https://owncloud.design/#/Design%20Tokens
} | ||
|
||
.margin-div { | ||
margin-left: 24px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class is not needed. You can use our margin classes. oc-ml
which uses the medium margin for **m**argin **l*eft
.
💥 Acceptance tests oc10-integration-notifications failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15512/
webUISharingNotifications-shareWithUsers-feature-53.pngwebUISharingNotificationsToRoot-shareWithUsers-feature-40.pngwebUISharingNotificationsToRoot-shareWithUsers-feature-53.png |
💥 Acceptance tests webUISharingNotifications failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15512/
webUISharingNotifications-shareWithUsers-feature-53.pngwebUISharingNotificationsToRoot-shareWithUsers-feature-40.pngwebUISharingNotificationsToRoot-shareWithUsers-feature-53.png |
💥 Acceptance tests IntegrationApp1 failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15512/
webUISharingAcceptSharesToRoot-acceptShares-feature-50.png |
💥 Acceptance tests webUISharingBasic failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15512/
webUISharingAcceptShares-acceptShares-feature-100.pngwebUISharingAcceptShares-acceptShares-feature-135.pngwebUISharingAcceptShares-acceptShares-feature-16.pngwebUISharingAcceptShares-acceptShares-feature-203.pngwebUISharingAcceptShares-acceptShares-feature-235.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-110.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-131.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-143.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-15.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-158.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-267.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-299.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-31.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-315.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-357.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-366.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-397.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-50.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-90.png |
@kulmann please check. By the way, in other vue files (for example FileLinkSidebar.vue, FileSharingSidebar.vue) there are also some pixel values for padding and margin. |
|
||
setShowDeclined(value) { | ||
showDeclined = value | ||
this.$forceUpdate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the this.$forceUpdate()
a mitigation for the change to showDeclined
not being reactive / causing re-rendering of the view? If that's the case then it would be better to add showDeclined
and showAllPending
to the data
prop after line 321. Then you don't need the getters and can skip the this.$forceUpdate()
call. Same applies for the other setter.
💥 Acceptance tests webUISharingNotifications failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15518/
webUISharingNotifications-shareWithUsers-feature-53.pngwebUISharingNotificationsToRoot-shareWithUsers-feature-40.pngwebUISharingNotificationsToRoot-shareWithUsers-feature-53.png |
💥 Acceptance tests oc10-integration-notifications failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15518/
webUISharingNotifications-shareWithUsers-feature-53.pngwebUISharingNotificationsToRoot-shareWithUsers-feature-40.pngwebUISharingNotificationsToRoot-shareWithUsers-feature-53.png |
💥 Acceptance tests IntegrationApp1 failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15518/
webUISharingAcceptSharesToRoot-acceptShares-feature-50.png |
💥 Acceptance tests webUISharingBasic failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15518/
webUISharingAcceptShares-acceptShares-feature-100.pngwebUISharingAcceptShares-acceptShares-feature-135.pngwebUISharingAcceptShares-acceptShares-feature-16.pngwebUISharingAcceptShares-acceptShares-feature-203.pngwebUISharingAcceptShares-acceptShares-feature-235.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-110.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-131.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-143.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-15.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-158.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-267.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-299.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-31.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-315.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-357.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-366.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-397.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-50.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-90.png |
💥 Acceptance tests webUISharingNotifications failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15557/
webUISharingNotifications-shareWithUsers-feature-53.pngwebUISharingNotificationsToRoot-shareWithUsers-feature-40.pngwebUISharingNotificationsToRoot-shareWithUsers-feature-53.png |
💥 Acceptance tests oc10-integration-notifications failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15557/
webUISharingNotifications-shareWithUsers-feature-53.pngwebUISharingNotificationsToRoot-shareWithUsers-feature-40.pngwebUISharingNotificationsToRoot-shareWithUsers-feature-53.png |
💥 Acceptance tests IntegrationApp1 failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15557/
webUISharingAcceptSharesToRoot-acceptShares-feature-50.png |
💥 Acceptance tests webUISharingBasic failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15557/
webUISharingAcceptShares-acceptShares-feature-100.pngwebUISharingAcceptShares-acceptShares-feature-135.pngwebUISharingAcceptShares-acceptShares-feature-16.pngwebUISharingAcceptShares-acceptShares-feature-203.pngwebUISharingAcceptShares-acceptShares-feature-235.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-110.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-131.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-143.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-15.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-158.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-267.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-299.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-31.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-315.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-357.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-366.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-397.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-50.pngwebUISharingAcceptSharesToRoot-acceptShares-feature-90.png |
.gitignore
Outdated
# Themes | ||
/packages/web-runtime/themes/** | ||
!/packages/web-runtime/themes/owncloud/** | ||
# # Themes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elizavetaRa @diocas please note that neither you nor us want the CERN theme in the ownCloud web master, esp. regarding licensing and logo files etc. Could you drop the respective commits from your fork in this PR? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not supposed to have happened. Those commits are only for our fork and are temporary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elizavetaRa please keep only your commits and rebase on the upstream, not on our fork.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No bad intent assumed ;) ping me/us if you need any suppor there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pascalwengerter in future we will do the pull requests from cernbox/web, I closed this and opened the corresponding one with newest changes: #5177
💥 Acceptance tests SharingFolderPermissionsGroups failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15839/
webUISharingFolderPermissionsGroups-sharePermissionsGroup-feature-61.pngwebUISharingFolderPermissionsGroups-sharePermissionsGroup-feature-62.pngwebUISharingFolderPermissionsGroups-sharePermissionsGroup-feature-63.pngwebUISharingFolderPermissionsGroups-sharePermissionsGroup-feature-64.pngwebUISharingFolderPermissionsGroups-sharePermissionsGroup-feature-65.pngwebUISharingFolderPermissionsGroups-sharePermissionsGroup-feature-66.png |
💥 Acceptance tests SharingFolderPermissionMultipleUsers failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15839/
webUISharingFolderPermissionMultipleUsers-shareFolderWithMultipleUsers-feature-61.pngwebUISharingFolderPermissionMultipleUsers-shareFolderWithMultipleUsers-feature-62.pngwebUISharingFolderPermissionMultipleUsers-shareFolderWithMultipleUsers-feature-63.pngwebUISharingFolderPermissionMultipleUsers-shareFolderWithMultipleUsers-feature-64.pngwebUISharingFolderPermissionMultipleUsers-shareFolderWithMultipleUsers-feature-65.pngwebUISharingFolderPermissionMultipleUsers-shareFolderWithMultipleUsers-feature-66.png |
💥 Acceptance tests webUISharingNotifications failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15839/
webUISharingNotifications-shareWithGroups-feature-24.pngwebUISharingNotifications-shareWithUsers-feature-21.pngwebUISharingNotifications-shareWithUsers-feature-32.pngwebUISharingNotifications-shareWithUsers-feature-40.pngwebUISharingNotifications-shareWithUsers-feature-53.pngwebUISharingNotificationsToRoot-shareWithGroups-feature-24.pngwebUISharingNotificationsToRoot-shareWithUsers-feature-19.pngwebUISharingNotificationsToRoot-shareWithUsers-feature-31.pngwebUISharingNotificationsToRoot-shareWithUsers-feature-40.pngwebUISharingNotificationsToRoot-shareWithUsers-feature-53.png |
💥 Acceptance tests Favorites failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15839/
webUIFavorites-favoritesFile-feature-110.pngwebUIFavorites-favoritesFile-feature-131.pngwebUIFavorites-favoritesFile-feature-14.pngwebUIFavorites-favoritesFile-feature-30.pngwebUIFavorites-favoritesFile-feature-46.pngwebUIFavorites-favoritesFile-feature-58.pngwebUIFavorites-favoritesFile-feature-65.pngwebUIFavorites-favoritesFile-feature-74.pngwebUIFavorites-favoritesFile-feature-80.pngwebUIFavorites-favoritesFile-feature-85.pngwebUIFavorites-unfavoriteFile-feature-104.pngwebUIFavorites-unfavoriteFile-feature-13.pngwebUIFavorites-unfavoriteFile-feature-34.pngwebUIFavorites-unfavoriteFile-feature-55.pngwebUIFavorites-unfavoriteFile-feature-72.pngwebUIFavorites-unfavoriteFile-feature-89.png |
💥 Acceptance tests SharingInternalGroups failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15839/
webUISharingInternalGroups-shareWithGroups-feature-100.pngwebUISharingInternalGroups-shareWithGroups-feature-182.pngwebUISharingInternalGroups-shareWithGroups-feature-19.pngwebUISharingInternalGroups-shareWithGroups-feature-196.pngwebUISharingInternalGroups-shareWithGroups-feature-216.pngwebUISharingInternalGroups-shareWithGroups-feature-228.pngwebUISharingInternalGroups-shareWithGroups-feature-245.pngwebUISharingInternalGroups-shareWithGroups-feature-260.pngwebUISharingInternalGroups-shareWithGroups-feature-271.pngwebUISharingInternalGroups-shareWithGroups-feature-293.pngwebUISharingInternalGroups-shareWithGroups-feature-324.pngwebUISharingInternalGroups-shareWithGroups-feature-325.pngwebUISharingInternalGroups-shareWithGroups-feature-349.pngwebUISharingInternalGroups-shareWithGroups-feature-350.pngwebUISharingInternalGroups-shareWithGroups-feature-72.pngwebUISharingInternalGroups-shareWithGroups-feature-73.pngwebUISharingInternalGroups-shareWithGroups-feature-74.png |
💥 Acceptance tests webUICreate failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/web/15839/
webUICreateFilesFolders-createFile-feature-13.pngwebUICreateFilesFolders-createFile-feature-19.pngwebUICreateFilesFolders-createFile-feature-25.pngwebUICreateFilesFolders-createFile-feature-31.pngwebUICreateFilesFolders-createFolderEdgeCases-feature-19.pngwebUICreateFilesFolders-createFolderEdgeCases-feature-20.pngwebUICreateFilesFolders-createFolderEdgeCases-feature-21.pngwebUICreateFilesFolders-createFolderEdgeCases-feature-22.pngwebUICreateFilesFolders-createFolderEdgeCases-feature-23.pngwebUICreateFilesFolders-createFolderEdgeCases-feature-24.pngwebUICreateFilesFolders-createFolderEdgeCases-feature-25.pngwebUICreateFilesFolders-createFolderEdgeCases-feature-26.pngwebUICreateFilesFolders-createFolderEdgeCases-feature-43.pngwebUICreateFilesFolders-createFolderEdgeCases-feature-44.pngwebUICreateFilesFolders-createFolderEdgeCases-feature-60.pngwebUICreateFilesFolders-createFolderEdgeCases-feature-61.pngwebUICreateFilesFolders-createFolderEdgeCases-feature-62.pngwebUICreateFilesFolders-createFolders-feature-12.pngwebUICreateFilesFolders-createFolders-feature-22.pngwebUICreateFilesFolders-createFolders-feature-27.pngwebUICreateFilesFolders-createFolders-feature-32.pngwebUICreateFilesFolders-createFolders-feature-39.pngwebUICreateFilesFolders-createFolders-feature-44.pngwebUICreateFilesFolders-createFolders-feature-49.png |
Description
The "Shared with me" view shows 2 different lists for pending and accepted shares. On request user can see the declined files (clicking "show declined shares).
Motivation and Context
General UX improvement: Motivate the user to accept or decline the shares through better overview of the statuses and highlighted pending section.
How Has This Been Tested?
UI test scenario: Sharing multiple files through one testing account with another
Types of changes
Checklist:
Open tasks:
General view
Declined section shown