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

feat: Add UI elements to modify navigation display #1295

Open
wants to merge 11 commits into
base: enh/1177/enable-nav-display-logic
Choose a base branch
from

Conversation

enjeck
Copy link
Contributor

@enjeck enjeck commented Aug 16, 2024

Part of #1177 and #1193

Todo

  • ensure it works when you update a context
  • ensure it works for owners and recipients too
  • Discuss the UI and copy
  • Ensure self-shares to the owner are deleted when share is deleted

@enjeck enjeck self-assigned this Aug 16, 2024
lib/Service/ShareService.php Outdated Show resolved Hide resolved
@enjeck
Copy link
Contributor Author

enjeck commented Aug 22, 2024

@nextcloud/designers Do you have any thoughts on how to go about the following?

We are trying to have settings where users can decide whether or not a Tables Application shows up in their navbar:
Screenshot from 2024-08-22 09-49-52

For the owners of the application, there are three possible default options to choose from. I feel like this could be reworded or made more succinct. See the "Navigation bar display" section in the image:
Screenshot from 2024-08-22 09-48-35

For share recipients, there's a checkbox. Not sure the wording or design is good enough:
Screenshot from 2024-08-22 09-49-31

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

@enjeck nice!

So unless I misunderstand something, the settings seem can be simplified. As admins are also just users, they can use the navigation preference like everyone else.

The main settings should only say:

[ ] Show in app list

As a single checkbox, no subheading needed. It can have a color-text-maxcontrast sentence below saying "This can be overridden by a per-account preference."

For the left navigation entry, same wording and setting

[ ] Show in app list

@enjeck
Copy link
Contributor Author

enjeck commented Aug 22, 2024

So unless I misunderstand something, the settings seem can be simplified. As admins are also just users, they can use the navigation preference like everyone else.

@jancborchardt This is not possible now, from what I see. There's no option for an owner to modify just their visibility, without affecting share recipients. And I'm not sure why it's not supported. I'll clarify at #1193 and see what can be changed

@juliusknorr juliusknorr added enhancement New feature or request 2. developing Work in progress labels Aug 29, 2024
@enjeck enjeck force-pushed the enh/1177/enable-nav-display-logic branch from ac42f69 to ccf873b Compare September 4, 2024 08:26
@enjeck enjeck force-pushed the enh/1177/enable-nav-display-logic branch from ccf873b to 8075a22 Compare September 19, 2024 09:51
@enjeck
Copy link
Contributor Author

enjeck commented Sep 24, 2024

So here are the challenges we've faced so far: Since navigation display setting is tied to shares, there is an additional share from the owner of an application to themselves. From discussions with @blizzz, we believe this is not good practice, but it's what it takes to get a better UX at the moment. The problem we have encountered now is that for owners of an application, the modifying the display does not work properly like with the sharees. So the plan now is:

  • when disabling, set values that are "shown to all" to "show to all, but me"
  • when enabled, set values that are "show to all, but me" to "show to all"

@enjeck enjeck marked this pull request as ready for review October 3, 2024 08:21
@enjeck enjeck force-pushed the enh/1177/enable-nav-display-logic branch 2 times, most recently from 0edd48a to cd00c05 Compare October 30, 2024 07:33
@enjeck enjeck force-pushed the enh/1177/enable-nav-display-logic branch from cd00c05 to bc6ae95 Compare November 19, 2024 07:08
@enjeck enjeck requested a review from blizzz as a code owner November 19, 2024 07:08
enjeck and others added 11 commits November 20, 2024 06:39
Signed-off-by: Cleopatra Enjeck M <patrathewhiz@gmail.com>
Signed-off-by: Cleopatra Enjeck M <patrathewhiz@gmail.com>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Cleopatra Enjeck M <patrathewhiz@gmail.com>
Signed-off-by: Cleopatra Enjeck M <patrathewhiz@gmail.com>
Signed-off-by: Cleopatra Enjeck M <patrathewhiz@gmail.com>
Signed-off-by: Cleopatra Enjeck M <patrathewhiz@gmail.com>
Signed-off-by: Cleopatra Enjeck M <patrathewhiz@gmail.com>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
… column

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress enhancement New feature or request
Projects
Status: 🏗️ In progress
Development

Successfully merging this pull request may close these issues.

4 participants