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 option to propagate permissions to sub collections #8233

Merged
merged 10 commits into from
Aug 7, 2018

Conversation

tlrobinson
Copy link
Contributor

@tlrobinson tlrobinson commented Aug 6, 2018

screenshot 2018-08-06 16 31 28

Also resolves #8205

@mazameli
Copy link
Contributor

mazameli commented Aug 7, 2018

Ideally I wouldn't see this toggle when dealing with a collection that doesn't have any subcollections.

screen shot 2018-08-06 at 4 54 47 pm

@mazameli
Copy link
Contributor

mazameli commented Aug 7, 2018

Here are the visual tweaks I made to the popover:

screen shot 2018-08-06 at 5 22 39 pm

@tlrobinson
Copy link
Contributor Author

Ideally I wouldn't see this toggle when dealing with a collection that doesn't have any subcollections.

Good point, fixed.

@mazameli
Copy link
Contributor

mazameli commented Aug 7, 2018

Looks like this is ready for review from @salsakran and @kdoh. Let's merge this puppy.

Copy link
Contributor

@salsakran salsakran left a comment

Choose a reason for hiding this comment

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

seems to work well from my manual testing of it

I can live with the toggle, though it does not fill me with aesthetic glee.

Copy link
Member

@kdoh kdoh left a comment

Choose a reason for hiding this comment

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

👍 for the propagation itself.

Not sure I'm loving the overall flow of the interaction for this though. The mixture of select items that immediately close the select and set your choice and then a toggle below just feels a little messy. For lack of a better term it feels like I could use more of a moment of pause to understand what going on if I'm potentially going to affect a bunch of items I can't see here (especially since we're defaulting to on).

Also curious if there's a reason why we went for a toggle vs a checkbox here. Small distinction but to me the toggle has more of a global implication whereas a checkbox feels possibly more appropriate on a per "instance" setting like this.

@mazameli
Copy link
Contributor

mazameli commented Aug 7, 2018

Cool, thanks for the feedback.

I could go either way with a checkbox vs. a toggle.

The reason that I opted for this defaulting on and it being handled by a toggle instead of a separate confirmation was:

  • the unanimous user feedback was that the expected default behavior would be for the setting to propagate down
  • because of that, I thought it would feel really clicky to have to confirm that expected behavior each time.

I.e., would you want to have to click this every time?

screen shot 2018-08-07 at 10 42 54 am

@tlrobinson tlrobinson merged commit a0d092c into master Aug 7, 2018
@tlrobinson tlrobinson deleted the propagate-collections-perms branch August 7, 2018 17:47
@kdoh
Copy link
Member

kdoh commented Aug 7, 2018

I think my reaction to it was more about the context in which things are happening than the defaulting to on (which makes sense). Just feels like the put these options in a popover strategy is starting to get a little messy. Anyhoo, as reflected by the fact this is merged I think it's an overall net win.

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.

Canceling changes in full collections permissions grid goes back instead of canceling
4 participants