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

implement shelves visibility #322

Merged
merged 13 commits into from Sep 2, 2022
Merged

implement shelves visibility #322

merged 13 commits into from Sep 2, 2022

Conversation

maxlath
Copy link
Member

@maxlath maxlath commented Jun 4, 2022

The client side of inventaire/inventaire#624

The trickiest bit was definitely the VisibilitySelector component and its inference system. Some examples:

  • if visibility = [ 'public' ] then checked = [ 'public', 'friends', 'groups', 'group:a', 'group:b' ]
  • if visibility = [ 'groups ] then checked = [ 'groups', 'group:a', 'group:b' ]
  • if group:a is unchecked, then both public and groups should be removed from the visibility array
    The good news is that this component should be straight forward to reuse for other document types (items, lists)

@maxlath maxlath requested a review from jum-s June 4, 2022 10:52
@maxlath maxlath added the shelves label Jun 4, 2022
@maxlath maxlath force-pushed the shelves-visibility branch 3 times, most recently from 43af79c to 2e47752 Compare August 6, 2022 10:04
@maxlath maxlath requested a review from jum-s August 6, 2022 18:09
maxlath added a commit that referenced this pull request Aug 7, 2022
jum-s pushed a commit that referenced this pull request Aug 15, 2022
@jum-s
Copy link
Contributor

jum-s commented Aug 27, 2022

the more i use this new visibility selector, the more a "private" checkbox makes sense to me. It would then be an explicit choice, instead of the currently implicit of not having any checkbox checked

could be v2, but i can dig into it

@maxlath
Copy link
Member Author

maxlath commented Aug 27, 2022

The problem with a "private" checkbox is that it breaks the selectors logic. The same purpose could be done without this inconvenient by haveing a "you" checkbox, which can not be unchecked, so that when you uncheck everything else, the only visibility remaining is "you". I would be in favor of shipping the current implementation and see the user feedback we get

@maxlath
Copy link
Member Author

maxlath commented Aug 30, 2022

Moreover, when every box is unchecked, the displayed selected value is "private", confirming what the user might have guessed
Capture d’écran du 2022-08-29 16-26-03

@jum-s
Copy link
Contributor

jum-s commented Aug 30, 2022

your last comment is largely depending of the information displayed by the consumer of the visibility selector, hence the user might not see "private" every time a selector is displayed

@maxlath maxlath merged commit 5c6bfd5 into master Sep 2, 2022
jum-s pushed a commit that referenced this pull request Sep 5, 2022
jum-s pushed a commit that referenced this pull request Sep 11, 2022
jum-s pushed a commit that referenced this pull request Sep 12, 2022
maxlath added a commit that referenced this pull request Sep 13, 2022
maxlath added a commit that referenced this pull request Sep 13, 2022
jum-s pushed a commit that referenced this pull request Sep 15, 2022
maxlath added a commit that referenced this pull request Sep 17, 2022
maxlath added a commit that referenced this pull request Sep 19, 2022
jum-s pushed a commit that referenced this pull request Sep 21, 2022
maxlath added a commit that referenced this pull request Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants