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 collaborators quick action #3585

Merged
merged 1 commit into from
Jun 17, 2020
Merged

Conversation

LukasHirt
Copy link
Collaborator

@LukasHirt LukasHirt commented Jun 9, 2020

Description

Added a new quick action which opens the new collaborators tab in the files list sidebar.

Related Issue

How Has This Been Tested?

  • test environment: Acceptance tests

Screenshots (if appropriate):

image

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:

  • Rebase after merging quick actions
  • Fix clicking on quick action when sidebar is already opened

@LukasHirt LukasHirt self-assigned this Jun 9, 2020
@LukasHirt
Copy link
Collaborator Author

This PR is built on top of #3573 After it will get merged I will rebase this PR.

@LukasHirt
Copy link
Collaborator Author

There is an issue that when we're switching highlighted file, the current tab and current panel resets. So if we click on the quick action while already having sidebar opened, we land in the default tab

One solution could be to set first the highlighted file to null which would do the reset before we set the new tab. This leads to a "jumpy" sidebar which quickly closes and opened => very ugly.

I'll try to see if there'd be some nicer solution. But I don't want to invest much time into debugging this since we'll change it later to overlay which will hide the quick actions anyway 🤷

@LukasHirt LukasHirt marked this pull request as ready for review June 16, 2020 09:45
@LukasHirt LukasHirt requested a review from kulmann June 16, 2020 09:45
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.

See comment. Also: please remember to bring back

const state = await filesList.isSharingButtonPresent(resource)
assert.ok(!state, `Error: Sharing button for resource ${resource} is not in disabled state`)

after merging https://github.com/owncloud/phoenix/pull/3586/files ;-) you made a comment there.

state.currentSidebarTab = tab
},

SET_CURRENT_COLLABORATORS_TAB_PANEL(state, panel) {
Copy link
Member

Choose a reason for hiding this comment

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

IMO this is too specific. What about an options object as an optional second argument for the current sidebar tab? Then we would be able to pass in context specific data without changing the store everytime we add something to the sidebar.

Could be called like this: SET_CURRENT_SIDEBAR_TAB(state, { tab, options: {'collaborators-tab-sub-panel': 'newCollaborator'} }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I still introduced mutation though which sets only options and not the whole tab object. This is because in collaborators we often change only the panel without changing the tab and it would feel strange to always have to define the collaborators' tab all over again.

@LukasHirt
Copy link
Collaborator Author

@kulmann I've brought back the commented out part of tests and introduced the options object. Pls, re-review.

@LukasHirt LukasHirt requested a review from kulmann June 16, 2020 18:26
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.

There is one bug: when I open the sidebar and then click on the link indicator of another row, the sidebar switches to the collaborators tab instead of the links tab... happens reliably in Chrome. Can you confirm that?

@@ -53,8 +53,8 @@ function openNewCollaboratorsPanel(ctx) {
})
}

function canShare(item) {
return item.canShare()
function canShare(ctx) {
Copy link
Member

Choose a reason for hiding this comment

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

If you want to have store and item as named variables, you can unwrap the ctx like this:
function canShare({ item, store }) {. Of course also fine like it is. 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, 🤦 I did it already automatically after doing it in the handlers of quickActions. I got rid of the object completely and just pass it in.

@LukasHirt
Copy link
Collaborator Author

There is one bug: when I open the sidebar and then click on the link indicator of another row, the sidebar switches to the collaborators tab instead of the links tab... happens reliably in Chrome. Can you confirm that?

Yes, happens to me as well. I'll jump to debugging first thing tomorrow morning

@LukasHirt
Copy link
Collaborator Author

Fixed the bug. Had to use setTimeout again because the beforeDestroy which resets the tab happened after the links tab has been set.

@LukasHirt LukasHirt requested a review from kulmann June 17, 2020 07:39
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.

🚀

A new quick action which opens the new collaborators panel inside of the files list sidebar
@LukasHirt LukasHirt merged commit 7ecd08f into master Jun 17, 2020
@LukasHirt LukasHirt deleted the feature/share-quick-action branch June 17, 2020 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Enhancement Add new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants