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

Move actions to sidebar #4255

Merged
merged 75 commits into from
Nov 16, 2020
Merged

Move actions to sidebar #4255

merged 75 commits into from
Nov 16, 2020

Conversation

LukasHirt
Copy link
Contributor

@LukasHirt LukasHirt commented Oct 30, 2020

Description

Moving resource actions from file list into the sidebar.

How Has This Been Tested?

  • test environment: Drone and manually

Screenshots (if appropriate):

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

Open tasks:

- [ ] Improve styles of buttons
- [ ] Adjust tests

  • Add changelog item
  • add folder-open icon from fontawesome for the navigate action
  • find an icon for the actions accordion item
  • unskip test that failed due to missing scrolling in actions dropdown

@LukasHirt LukasHirt self-assigned this Oct 30, 2020
@update-docs

This comment has been minimized.

@LukasHirt LukasHirt marked this pull request as ready for review November 13, 2020 10:30
@LukasHirt
Copy link
Contributor Author

Skipping the failing scenario because it's actually an issue in master as well #4310 tested both locally and on ocis.owncloud.works. Not sure at all how the test could pass in other PRs then 🤷

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.

The code itself looks good and the consistency of having everything in the right sidebar is super nice now.
Unfortunately I found some beef on this PR while trying it out again locally.

"shared with me" page:

  • There is a "download" action for folders
  • "mark as favorite" doesn't work
  • if status of share = pending -> we shouldn't allow anything! maybe we can just hide the accordion entirely and display a "please accept or decline this share" message?
  • "rename" action should only be available if the sharing role permits it

"shared with others" page:

  • file details (top row above the accordion) looks different: missing favorites-star, missing file details
  • folders have a "Download" action. Should not be available.

"deleted files" page:

  • actions accordion item is not collapsible. Stays open on click.

Also, we wanted to re-activate a test scenario that was disabled in a previous PR (issue with scrolling in actions dropdown). Added a checkbox to the PR description for this.

@kulmann
Copy link
Member

kulmann commented Nov 13, 2020

Screenshot 2020-11-13 at 15 13 13

I added a commit that does two tiny things (see screenshot). Please revert the commit if you don't agree. But we should at least find an icon for the actions accordion item, so that all the accordion items have one.
a) Set an icon for the actions accordion item. Please check if you agree with the icon. A "play button" makes a reasonable impression for me.
b) Added a slight horizontal padding for the contents of the accordion items. Without it I had a pretty hard time to find a visible separation between the respective accordion items.

@LukasHirt
Copy link
Contributor Author

Pushed a new commit yesterday which tackles those bugs @kulmann Some notes though:

"mark as favorite" doesn't work

Removed this action from shared lists since we do not have starred value when building resources there. I would see this out of the scope of this PR to tackle that.

"rename" action should only be available if the sharing role permits it

Rename should be always available. Renaming shared resource shouldn't rename the original one but only the local one for the user.

actions accordion item is not collapsible. Stays open on click.

Was happening everywhere because sometimes the opened accordion was set to null and open-first took care of opening the accordion which resulted on trying to change opened-accordion-id from null to null. Fixed as well.

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.

Awesome, thanks for adjusting!

More small things that I found:
1: "shared with others" and "shared with me" pages

  • when you select multiple files, there should only be the Delete batch action. Please remove the Copy and Move batch actions there.

2: "shared with others" page

  • I can't reproduce it, but I had an issue navigating into a folder from the ride sidebar of this page.

3: "shared with me" page

  • when I open the right sidebar for a folder that was shared with me without reshare permissions, I still see the "Add people" button in the right sidebar. Same for creating public links. The backend fails correctly, so I get an error message upon sharing. The "All files" page shows a "You don't have permission to share this folder." message instead of the button. We should do the same on the "shared with me" page if possible.

4: we wanted to re-enable a test that was disabled in a recent PR. about scrolling issues when the actions dropdown contained too many actions - in the right sidebar we have scrolling, so that should work again now.

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.

Yeeeees, awesome! 🚀 👍 🥇

@kulmann kulmann merged commit 1dc2627 into master Nov 16, 2020
@delete-merged-branch delete-merged-branch bot deleted the move-actions-to-sidebar branch November 16, 2020 16:57
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.

Share received inside share_folder cannot be unshared
3 participants