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.

Sign up for GitHub

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

Jump to bottom

feat(Contexts): enable nav bar display logic #1193

Merged
merged 25 commits into from
Jan 10, 2025
Merged

feat(Contexts): enable nav bar display logic #1193

merged 25 commits into from
Jan 10, 2025

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Jul 12, 2024 •
edited
Loading

contributes to #1177

It was temporarily disabled and shown by default as we did not have web UI controls to configure it.

@blizzz blizzz added enhancement New feature or request 2. developing Work in progress labels Jul 12, 2024
@blizzz blizzz mentioned this pull request Jul 12, 2024
Closed
@blizzz blizzz self-assigned this Jul 17, 2024
@blizzz blizzz force-pushed the enh/1177/enable-nav-display-logic branch from 2a6a88f to 52638ee Compare July 22, 2024 22:10
@blizzz blizzz marked this pull request as ready for review July 22, 2024 22:11
@blizzz blizzz added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 22, 2024
@blizzz blizzz requested review from juliusknorr, elzody and enjeck July 22, 2024 22:11
juliusknorr
juliusknorr approved these changes Jul 23, 2024
View reviewed changes
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Backend change looks good 👍

@enjeck enjeck force-pushed the enh/1177/enable-nav-display-logic branch from 52638ee to ac42f69 Compare August 6, 2024 18:45
juliusknorr
juliusknorr requested changes Aug 6, 2024
View reviewed changes
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Let me block the merge just to be sure this doesn't get lost from the initial post

⚠️ merge only with the frontend tasks as laid out in #1177 (comment)

@enjeck
Copy link
Contributor

enjeck commented Aug 6, 2024

Let me block the merge just to be sure this doesn't get lost from the initial post

⚠️ merge only with the frontend tasks as laid out in #1177 (comment)

Yeah, I'm looking into the frontend parts :D

@juliusknorr
Copy link
Member

Great, thanks :)

@enjeck enjeck mentioned this pull request Aug 16, 2024
Merged
4 tasks
@enjeck
Copy link
Contributor

enjeck commented Aug 22, 2024

It seems there's no way for an owner to modify their navigation entry if there are no shares? Creating an application without shares means there's no navigation entry, and no way to change it. @blizzz

@blizzz
Copy link
Member Author

blizzz commented Sep 2, 2024

It seems there's no way for an owner to modify their navigation entry if there are no shares? Creating an application without shares means there's no navigation entry, and no way to change it. @blizzz

This is accurate. We bound the nav bar display state to sharing. Also the share id is part of the primary key.

I do see it is reasonable to have this setting for non-shared applications, but spontaneously a quick solution that is not too terrible does not come to my mind. Is it a blocker?

@enjeck
Copy link
Contributor

enjeck commented Sep 2, 2024

It seems there's no way for an owner to modify their navigation entry if there are no shares? Creating an application without shares means there's no navigation entry, and no way to change it. @blizzz

This is accurate. We bound the nav bar display state to sharing. Also the share id is part of the primary key.

I do see it is reasonable to have this setting for non-shared applications, but spontaneously a quick solution that is not too terrible does not come to my mind. Is it a blocker?

Maybe not a blocker, but still inconvenient and not very user-friendly. The UI right now doesn't look great, and I can't seem to fix it due to this: #1295

@blizzz
Copy link
Member Author

blizzz commented Sep 2, 2024

It seems there's no way for an owner to modify their navigation entry if there are no shares? Creating an application without shares means there's no navigation entry, and no way to change it. @blizzz

This is accurate. We bound the nav bar display state to sharing. Also the share id is part of the primary key.
I do see it is reasonable to have this setting for non-shared applications, but spontaneously a quick solution that is not too terrible does not come to my mind. Is it a blocker?

Maybe not a blocker, but still inconvenient and not very user-friendly. The UI right now doesn't look great, and I can't seem to fix it due to this: #1295

Yup, I am with you there. Need to think about a good strategy.

@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 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
@blizzz

This comment was marked as outdated.

Sign in to view
@enjeck enjeck force-pushed the enh/1177/enable-nav-display-logic branch from 99b8922 to 40a2485 Compare November 27, 2024 09:14
@enjeck enjeck force-pushed the enh/1177/enable-nav-display-logic branch from 40a2485 to c144b4e Compare December 16, 2024 06:25
@blizzz @enjeck
feat(Contexts): enable nav bar display logic
5617fe6 
It was temporarily disabled and shown by default as we did not have web UI
controls to configure it.

Signed-off-by: Arthur Schiwon <[email protected]>
blizzz and others added 5 commits January 3, 2025 04:45
@blizzz @enjeck
fix: apply target userId when setting display mode
aeb83f0 
Signed-off-by: Arthur Schiwon <[email protected]>
@blizzz @enjeck
fix: have visibility checks already in SQL instead of later filtering
eca5cf5 
Signed-off-by: Arthur Schiwon <[email protected]>
@enjeck
fix(Cypress): remove check for navbar header
4a690c7 
Signed-off-by: Cleopatra Enjeck M. <[email protected]>
@blizzz @enjeck
fix: treat owners like other users
265c77e 
After frontend changes we do not need the intermediate RECIPIENT-ONLY mode
any more. And so this fixes setting the override for owners.

Signed-off-by: Arthur Schiwon <[email protected]>
@blizzz @enjeck
feat(Contexts): consider group shares when determining nav display
84e26e5 
Signed-off-by: Arthur Schiwon <[email protected]>
@enjeck enjeck force-pushed the enh/1177/enable-nav-display-logic branch from 447e4a7 to 84e26e5 Compare January 3, 2025 03:45
enjeck and others added 19 commits January 4, 2025 15:16
@enjeck
feat: add UI elements to change nav display
7f8da4a 
Signed-off-by: Cleopatra Enjeck M <[email protected]>
@enjeck
fix: update context navigation display elements
afcbe45 
Signed-off-by: Cleopatra Enjeck M <[email protected]>
@blizzz @enjeck
fix(Context): fix wrong parameters to permission check
e05745a 
Signed-off-by: Arthur Schiwon <[email protected]>
@blizzz @enjeck
fix(DB): override mapper::update() where tables has no id column
6a693aa 
Signed-off-by: Arthur Schiwon <[email protected]>
@enjeck
fix: use NcDialog
04cd53e 
Signed-off-by: Cleopatra Enjeck M <[email protected]>
@enjeck
fix: properly update navigation display via checkbox
41b5b24 
Signed-off-by: Cleopatra Enjeck M <[email protected]>
@enjeck
enh: add share to oneself to control context nav display
4be1a7c 
Signed-off-by: Cleopatra Enjeck M <[email protected]>
@enjeck
enh: use checkbox for default nav display mode
bd37daf 
Signed-off-by: Cleopatra Enjeck M <[email protected]>
@enjeck
enh: modify display mode update for owner
1ab79e7 
Signed-off-by: Cleopatra Enjeck M <[email protected]>
@blizzz @enjeck
fix(Navigation): use default display mode when there is no user override
b00f1c0 
Signed-off-by: Arthur Schiwon <[email protected]>
@blizzz @enjeck
fix(DB): NavigationMapper has to override insert due to not having an id
454844e 
… column

Signed-off-by: Arthur Schiwon <[email protected]>
@enjeck
fix: update display mode when editing context
efab19c 
Signed-off-by: Cleopatra Enjeck M. <[email protected]>
@blizzz @enjeck
enh: set visibility also when context is provided by group shares
f4f3277 
- removed to only fetch the one share that matches the user id. Group
  shares my be present and only those relevant to the current user are
  being fetched.
- setting the override now against any share. There might be more and some
  might be dropped – the override is working against any of them
- one none-hidden setting wins
- dropped using the NAV_ENTRY_MODE_RECIPIENTS as it is not needed anymore

Signed-off-by: Arthur Schiwon <[email protected]>
@blizzz @enjeck
feat(Contexts): dynamic navigation bar update on config toggle
4ab44af 
Signed-off-by: Arthur Schiwon <[email protected]>
@enjeck
enh: remove usages of NAV_ENTRY_MODE_RECIPIENTS
e1e3737 
Signed-off-by: Cleopatra Enjeck M. <[email protected]>
@enjeck
fix: add copyright and update openapi
30748c4 
Signed-off-by: Cleopatra Enjeck M. <[email protected]>
@blizzz
ci: suppress psalm warning against private NavigationController class
c925f42 
this can be removed once >=NC31 is supported and our NavigationController
is adjusted

Signed-off-by: Arthur Schiwon <[email protected]>
@juliusknorr
fix: Make labels translatable
4d5a063 
Signed-off-by: Julius Knorr <[email protected]>
@blizzz
Merge pull request #1295 from nextcloud/nav-display-frontend
76b51d8 
feat: Add UI elements to modify navigation display
@blizzz blizzz requested a review from juliusknorr January 9, 2025 21:05
@blizzz
Copy link
Member Author

blizzz commented Jan 9, 2025

Remaining one: #1250, it should go together with this PR

@blizzz blizzz merged commit 4378307 into main Jan 10, 2025
54 checks passed
@blizzz blizzz deleted the enh/1177/enable-nav-display-logic branch January 10, 2025 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliusknorr juliusknorr juliusknorr approved these changes

@elzody elzody Awaiting requested review from elzody

@enjeck enjeck Awaiting requested review from enjeck enjeck is a code owner

Assignees

@blizzz blizzz

Labels
3. to review Waiting for reviews enhancement New feature or request
Projects
📝 Office team
Status: ☑️ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@blizzz @enjeck @juliusknorr