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

BugFix - Icon Visibilities #14030

Merged
merged 17 commits into from
Nov 25, 2024
Merged

BugFix - Icon Visibilities #14030

merged 17 commits into from
Nov 25, 2024

Conversation

alperozturk96
Copy link
Collaborator

@alperozturk96 alperozturk96 commented Nov 15, 2024

  • Tests written, or not not needed

• Hide the people icon after sharing with at least one user (resolves share icon visibility issues).
• Use correct overlay icons for folders (e.g., shared with users, shared via link, locked, encrypted).
• Add offline operation folder type to screenshot tests.
• Fix and update broken screenshot tests.

Before After
w r

Folders With Overlay Icon

Screenshot 2024-11-15 at 11 57 50

@AndyScherzinger
Copy link
Member

Screenshot Tests look good to me. Just replace the existing ones with the new ones. They are visually similar but fail due to the update of the common lib which has m3 theming so the blue and Grey tones are slightly different now 👍

@AndyScherzinger
Copy link
Member

LGTM

@alperozturk96 As a follow up maybe, did you see that the folder icons with an overlay are slightly smaller see their width than folders without an overlay? 🔍

@alperozturk96 alperozturk96 marked this pull request as draft November 15, 2024 15:45
@alperozturk96
Copy link
Collaborator Author

@AndyScherzinger Thank you. Good point. I will update the PR.

Copy link
Collaborator

@ZetaTom ZetaTom left a comment

Choose a reason for hiding this comment

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

During my testing I've noticed some inconsistencies compared to the previous implementation. Otherwise it seems to work as expected; the share icon is only displayed when expected.

  1. Corners of thumbnails are no longer rounded (see screenshot below)
  2. Thumbnails of images are only displayed after a manual reload (pull to refresh)
  3. The ic_movie overlay for video files is scaled wrong (see screenshot below)
  4. Appearance of auto-upload folder has changed (icon used to be taller)

image

@alperozturk96 alperozturk96 force-pushed the bugfix/icon-visibilities branch 2 times, most recently from 9dad42b to 6a12b2b Compare November 20, 2024 10:59
@alperozturk96
Copy link
Collaborator Author

alperozturk96 commented Nov 20, 2024

During my testing I've noticed some inconsistencies compared to the previous implementation. Otherwise it seems to work as expected; the share icon is only displayed when expected.

1. Corners of thumbnails are no longer rounded (see screenshot below)

2. Thumbnails of images are only displayed after a manual reload (pull to refresh)

3. The `ic_movie` overlay for video files is scaled wrong (see screenshot below)

4. Appearance of auto-upload folder has changed (icon used to be taller)

image

I uploaded the image and got a preview immediately. I’m unable to reproduce the second issue. Is it the same on the master branch? If not, could you provide a video recording so I can attempt to reproduce it?

Demo for thumbnail

thumn.mp4

The appearance of all folder overlay icons has changed due to updated logic. The width of overlayed icons was not the same as non-overlayed icons. With this PR, the widths are now consistent, which will affect the appearance.

For mentioned other issues, I updated the PR.

Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
alperozturk96 and others added 8 commits November 21, 2024 09:13
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Copy link

Codacy

Lint

TypemasterPR
Warnings5959
Errors33

SpotBugs

CategoryBaseNew
Bad practice6565
Correctness5858
Dodgy code298298
Experimental11
Internationalization77
Malicious code vulnerability11
Multithreaded correctness77
Performance5353
Security1818
Total508508

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.

Just a small thing – the avatar seems to be a bit off to the right compared to the sharing icons. Not super important but might be easy to fix?
image

@tobiasKaminsky tobiasKaminsky merged commit d4b4689 into master Nov 25, 2024
19 of 21 checks passed
@tobiasKaminsky tobiasKaminsky deleted the bugfix/icon-visibilities branch November 25, 2024 13:28
@tobiasKaminsky
Copy link
Member

Just a small thing – the avatar seems to be a bit off to the right compared to the sharing icons. Not super important but might be easy to fix?

Will be done in a new PR.

@AndyScherzinger AndyScherzinger added this to the Nextcloud App 3.31.0 milestone Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants