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

Updates to new sharing flow #40629

Merged
merged 2 commits into from Oct 2, 2023
Merged

Updates to new sharing flow #40629

merged 2 commits into from Oct 2, 2023

Conversation

Fenn-CS
Copy link
Contributor

@Fenn-CS Fenn-CS commented Sep 26, 2023 •
edited

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

@Fenn-CS Fenn-CS mentioned this pull request Sep 26, 2023
Closed
7 tasks
@Fenn-CS Fenn-CS changed the title WIP : Further polish sharing flow Updates to new sharing flow Sep 26, 2023
@Fenn-CS Fenn-CS marked this pull request as ready for review September 26, 2023 21:05
@Fenn-CS
Copy link
Contributor Author

Fenn-CS commented Sep 26, 2023

/compile amend /

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

Fenn-CS 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

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?

@Fenn-CS
Copy link
Contributor Author

Fenn-CS 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

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
@Fenn-CS
Copy link
Contributor Author

Fenn-CS commented Sep 28, 2023

Thanks for the clarification @jancborchardt...

Both your concerns have been addressed.

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

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
@Fenn-CS Fenn-CS force-pushed the 40438-sharing-cleanup branch 2 times, most recently from 929df20 to d209b56 Compare September 28, 2023 13:55
@Fenn-CS Fenn-CS force-pushed the 40438-sharing-cleanup branch 2 times, most recently from 3e27e73 to 4360dd7 Compare September 29, 2023 17:20
@Fenn-CS
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>
@Fenn-CS
Copy link
Contributor Author

Fenn-CS commented Oct 2, 2023

/compile amend /

@Fenn-CS @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>
@Fenn-CS Fenn-CS mentioned this pull request Oct 2, 2023
Merged
@Fenn-CS Fenn-CS merged commit 8c2ff08 into master Oct 2, 2023
40 checks passed
@Fenn-CS Fenn-CS 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

@juliushaertl juliushaertl Awaiting requested review from juliushaertl

@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
@Fenn-CS @jancborchardt @artonge @blizzz