Skip to content

Updates to new sharing flow#40629

Merged
nfebe merged 2 commits intomasterfrom
40438-sharing-cleanup
Oct 2, 2023
Merged

Updates to new sharing flow#40629
nfebe merged 2 commits intomasterfrom
40438-sharing-cleanup

Conversation

@nfebe
Copy link
Contributor

@nfebe nfebe commented Sep 26, 2023 •
edited
Loading

Resolves : #40438, #40395, #40612

Dark mode

Before Now
Screenshot from 2023-09-26 12-13-19 Screenshot from 2023-09-26 09-25-44

Footer background

Before Now
Screenshot from 2023-09-26 12-11-29 Screenshot from 2023-09-26 12-08-16

@nfebe nfebe force-pushed the 40438-sharing-cleanup branch from 57f5bdf to f1b3336 Compare September 26, 2023 11:31
@nfebe nfebe mentioned this pull request Sep 26, 2023
Closed
7 tasks
@nfebe nfebe force-pushed the 40438-sharing-cleanup branch from f1b3336 to bfcaddd Compare September 26, 2023 21:05
@nfebe nfebe changed the title WIP : Further polish sharing flow Updates to new sharing flow Sep 26, 2023
@nfebe nfebe marked this pull request as ready for review September 26, 2023 21:05
@nfebe
Copy link
Contributor Author

nfebe commented Sep 26, 2023

/compile amend /

@nfebe nfebe added this to the Nextcloud 27.1.2 milestone Sep 26, 2023
@nfebe
Copy link
Contributor Author

nfebe commented Sep 26, 2023

Follow up

Item Action
The "Cancel" and primary button on the bottom need a backdrop to separate them a bit from the background, best a fade from color-main-background on the bottom to transparent on the top. Done see screenshots above
Currently links are sorted below emails once created, but sorting should be: Links, mails, users It is the case
If allow public upload admin setting is unchecked we should rename "Allow upload and editing" to "allow editing" and file drop / create permission should be disabled state Done
The content of the sidebar is scroll-able when it should only be the content of the share tab Done
It is confusing to have the download permission under "Custom permissions" as it is not taken into account for bundles Moved up, and it is now just an advanced setting
Do not show unnecessary password expired message #40493 Done
Others Dark mode corrections, see screenshot above/below

Dark mode view

Screenshot from 2023-09-26 21-15-30

@nfebe nfebe requested a review from szaimen September 26, 2023 21:26
jancborchardt
jancborchardt reviewed Sep 27, 2023
View reviewed changes
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.

Nice improvements! Some feedback:

  • The gradient for the bottom buttons should be color-main-background on the bottom, currently it seems to use primary-light. It should not really be actively visible.
  • The gradient needs to extend to the full left and right of the sidebar and also to whichever much bottom padding the buttons have. Currently it starts directly at the buttons, which will lead to things looking cut off once you scroll.
  • The "Delete share" button also has a gradient on it for some reason?

@nfebe
Copy link
Contributor Author

nfebe commented Sep 27, 2023

The gradient for the bottom buttons should be color-main-background on the bottom, currently it seems to use primary-light. It should not really be actively visible.

Yeah, this was mentioned from the original recommendation but I realized color-main-background makes not visual difference as it matches with the main background of the color theme. So can you please provide more context for me to understand the purpose of a non-visible gradient?

The gradient needs to extend to the full left and right of the sidebar and also to whichever much bottom padding the buttons have. Currently it starts directly at the buttons, which will lead to things looking cut off once you scroll.

I don't think it would be easy to achieve that, as everything is aligned inside the a container to keep the left-aligned items in place.

The "Delete share" button also has a gradient on it for some reason?

On master "Delete share" has no bg, unlike with 27, so I added a similar gradient. Could easily revert this.

cc: @jancborchardt

@jancborchardt
Copy link
Member

jancborchardt commented Sep 27, 2023 •
edited
Loading

The gradient for the bottom buttons should be color-main-background on the bottom, currently it seems to use primary-light. It should not really be actively visible.

Yeah, this was mentioned from the original recommendation but I realized color-main-background makes not visual difference as it matches with the main background of the color theme. So can you please provide more context for me to understand the purpose of a non-visible gradient?

Yep, here you go :) The gradient is invisible, yes, but it becomes visible when the content is big enough to make the container scroll (or the viewport is small enough).
If additional separation is needed, we could add an additional 2px border of color-main-background to the buttons.
Note the screenshots are based on the old mockups, so disregard any of that, only the gradient. :)

Current situation when scrolled With the gradient
image image

The gradient needs to extend to the full left and right of the sidebar and also to whichever much bottom padding the buttons have. Currently it starts directly at the buttons, which will lead to things looking cut off once you scroll.

I don't think it would be easy to achieve that, as everything is aligned inside the a container to keep the left-aligned items in place.

Right, actually if the gradient is color-main-background it is also not necessary to do that for left and right. For the bottom it is necessary though and could be done via negative margin?

The "Delete share" button also has a gradient on it for some reason?

On master "Delete share" has no bg, unlike with 27, so I added a similar gradient. Could easily revert this.

"Delete share" could just be a tertiary button as last element of the content, not as part of the button section which would then only have "Cancel" and "Update share".

@blizzz blizzz mentioned this pull request Sep 27, 2023
Merged
@nfebe nfebe force-pushed the 40438-sharing-cleanup branch from bfcaddd to a962925 Compare September 28, 2023 10:35
@nfebe
Copy link
Contributor Author

nfebe commented Sep 28, 2023

Thanks for the clarification @jancborchardt...

Both your concerns have been addressed.

Screenshot from 2023-09-28 11-35-10

@nfebe nfebe requested a review from jancborchardt September 28, 2023 10:37
jancborchardt
jancborchardt approved these changes Sep 28, 2023
View reviewed changes
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.

Looks good!

This was linked to issues Sep 28, 2023
[Bug]: "Set expiration date" in share preview is not set when admin has set a default expiration date #40612
Closed
new share dropdown not good visible in dark mode #40395
Closed
@nfebe nfebe force-pushed the 40438-sharing-cleanup branch from 8862b3b to 34d3624 Compare September 28, 2023 12:49
@nfebe nfebe requested a review from sorbaugh September 28, 2023 12:51
@nfebe nfebe force-pushed the 40438-sharing-cleanup branch from 34d3624 to 890e8ce Compare September 28, 2023 12:51
@nfebe nfebe force-pushed the 40438-sharing-cleanup branch 2 times, most recently from 929df20 to d209b56 Compare September 28, 2023 13:55
@nfebe nfebe force-pushed the 40438-sharing-cleanup branch 2 times, most recently from 3e27e73 to 4360dd7 Compare September 29, 2023 17:20
@nfebe
Updates to sharing flow
a092bae 
- Show enforced expiry date for new shares.
- Improve quick share dropdown visibility in dark mode.
- Prevent expiry date from showing expire for incoming shares.
by updating the check for `share.passwordExpirationTime` to equally
check for `undefined`.
- Move "Download permission/attribute" from custom setting (as it is just
another advanced setting and not an actual permission).
- Show correct text for upload/editing when "allow public uploads" is enabled
or disabled by admin.

Signed-off-by: fenn-cs <fenn25.fn@gmail.com>
@nfebe nfebe force-pushed the 40438-sharing-cleanup branch from 4360dd7 to 3ae7cd2 Compare October 2, 2023 10:59
@nfebe
Copy link
Contributor Author

nfebe commented Oct 2, 2023

/compile amend /

@nfebe @nextcloud-command
Correct check for enabled expiration date
0537bbd 
The default expiration date cannot be enforced if it is not enabled
in the first place. Hence, the check for whether a share has an expiration date
should not consider enforcements but only where the share expiry type is enabled.

For example : Using `this.config.isDefaultExpireDateEnabled` instead of
`this.config.isDefaultExpireDateEnforced` which can be verified by checking `isExpiryDateEnforced`.

Resolves : #40612

Signed-off-by: fenn-cs <fenn25.fn@gmail.com>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@nfebe nfebe mentioned this pull request Oct 2, 2023
Merged
@nfebe nfebe merged commit 8c2ff08 into master Oct 2, 2023
@nfebe nfebe deleted the 40438-sharing-cleanup branch October 2, 2023 18:26
@thgoebel thgoebel mentioned this pull request Oct 28, 2023
Closed
8 tasks
@Ornanovitch Ornanovitch mentioned this pull request Nov 12, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jancborchardt jancborchardt jancborchardt approved these changes

@artonge artonge artonge approved these changes

@susnux susnux Awaiting requested review from susnux

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@Pytal Pytal Awaiting requested review from Pytal

@szaimen szaimen Awaiting requested review from szaimen

@sorbaugh sorbaugh Awaiting requested review from sorbaugh

Assignees

No one assigned

Labels

27-feedback feature: sharing

Projects

📁 Files team
Archived in project

Milestone

Nextcloud 27.1.3

4 participants

@nfebe @jancborchardt @artonge @blizzz