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

Fix tray styling #5715

Merged
merged 30 commits into from
May 30, 2023
Merged

Fix tray styling #5715

merged 30 commits into from
May 30, 2023

Conversation

claucambra
Copy link
Collaborator

@claucambra claucambra commented May 19, 2023

Addresses the issues caused by #5377

Principally:

  • Simplifies code
  • Returns two-line activity titles to improve readability
  • Fixes clipping of avatars
  • Moves file details button to first row to conserve space
  • Uses our custom button components for consistency
  • Fixes dark mode issues
  • Properly aligns activity text when secondary text is not there
  • more

After changes:

Screen.Recording.2023-05-19.at.23.24.13.mov
Screenshot 2023-05-23 at 21 50 27 Screenshot 2023-05-23 at 21 50 53

Before changes:

Screen.Recording.2023-05-19.at.18.07.09.mov
Screenshot 2023-05-23 at 21 52 07 Screenshot 2023-05-23 at 21 52 21

@claucambra claucambra self-assigned this May 19, 2023
@codecov
Copy link

codecov bot commented May 19, 2023

Codecov Report

Merging #5715 (0acc015) into master (867fae1) will increase coverage by 0.10%.
The diff coverage is n/a.

❗ Current head 0acc015 differs from pull request most recent head 4c096a8. Consider uploading reports for the commit 4c096a8 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5715      +/-   ##
==========================================
+ Coverage   60.26%   60.37%   +0.10%     
==========================================
  Files         143      143              
  Lines       18518    18518              
==========================================
+ Hits        11160    11180      +20     
+ Misses       7358     7338      -20     

see 5 files with indirect coverage changes

@claucambra
Copy link
Collaborator Author

/backport to stable-3.9

src/gui/main.cpp Outdated Show resolved Hide resolved
src/gui/tray/NCButtonContents.qml Outdated Show resolved Hide resolved
theme/Style/Style.qml Outdated Show resolved Hide resolved
src/gui/tray/ActivityItemContent.qml Show resolved Hide resolved
src/gui/tray/ActivityItemContent.qml Show resolved Hide resolved
@mgallien
Copy link
Collaborator

@claucambra can you provide screenshot before/after ?

@claucambra
Copy link
Collaborator Author

@claucambra can you provide screenshot before/after ?

Attached now 🙂

@claucambra
Copy link
Collaborator Author

claucambra commented May 23, 2023

dark mode issues when fusion is enabled:
Screenshot 2023-05-23 at 21 58 06

@claucambra claucambra force-pushed the bugfix/fix-activities branch 3 times, most recently from 77ee5f5 to f06da12 Compare May 23, 2023 15:50
@claucambra claucambra changed the title Fix activities Fix tray styling May 23, 2023
@claucambra
Copy link
Collaborator Author

dark mode issues when fusion is enabled: Screenshot 2023-05-23 at 21 58 06

now fixed

@claucambra claucambra requested a review from mgallien May 24, 2023 06:59
@mgallien
Copy link
Collaborator

@claucambra can you solve the conflicts ?

@claucambra claucambra force-pushed the bugfix/fix-activities branch 2 times, most recently from 2eb6935 to 575b864 Compare May 24, 2023 11:12
@claucambra
Copy link
Collaborator Author

@claucambra can you solve the conflicts ?

Yep, now fixed

Copy link
Collaborator

@mgallien mgallien left a comment

Choose a reason for hiding this comment

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

dark mode works fine !!!! super cool
see my inline comments
including relying on implicit magic behavior of our components inheriting existing ones from Qt modules
given the amount of visual changes, I think you should request a design review on this

src/gui/ConflictItemFileInfo.qml Outdated Show resolved Hide resolved
src/gui/ConflictItemFileInfo.qml Outdated Show resolved Hide resolved
src/gui/tray/ActivityItemContent.qml Show resolved Hide resolved
@claucambra
Copy link
Collaborator Author

claucambra commented May 25, 2023

dark mode works fine !!!! super cool

Thanks :)

see my inline comments including relying on implicit magic behavior of our components inheriting existing ones from Qt modules

I am not convinced by the magic behaviour as the purpose of the CustomButton is to abstract broad visual behaviour so as to not have to repeat it everywhere throughout our code -- that is to say, we shouldn't have to implement things like a hover effect everywhere, which relies on hoverEnabled: true already being set in CustomButton

@claucambra claucambra force-pushed the bugfix/fix-activities branch 2 times, most recently from d5a3848 to f8285f5 Compare May 25, 2023 04:30
@mgallien
Copy link
Collaborator

dark mode works fine !!!! super cool

Thanks :)

see my inline comments including relying on implicit magic behavior of our components inheriting existing ones from Qt modules

I am not convinced by the magic behaviour as the purpose of the CustomButton is to abstract broad visual behaviour so as to not have to repeat it everywhere throughout our code -- that is to say, we shouldn't have to implement things like a hover effect everywhere, which relies on hoverEnabled: true already being set in CustomButton

if what you want to achieve is that by default all instances of CustomButton do have hover effect enabled then you can just set a default value for the property inside CustomButton
if someone wants to use the component it may decide to change the value of the property and currently that would have no effect looking like a bug (and I would consider this a bug)

@claucambra
Copy link
Collaborator Author

dark mode works fine !!!! super cool

Thanks :)

see my inline comments including relying on implicit magic behavior of our components inheriting existing ones from Qt modules

I am not convinced by the magic behaviour as the purpose of the CustomButton is to abstract broad visual behaviour so as to not have to repeat it everywhere throughout our code -- that is to say, we shouldn't have to implement things like a hover effect everywhere, which relies on hoverEnabled: true already being set in CustomButton

if what you want to achieve is that by default all instances of CustomButton do have hover effect enabled then you can just set a default value for the property inside CustomButton if someone wants to use the component it may decide to change the value of the property and currently that would have no effect looking like a bug (and I would consider this a bug)

Okay, I declared it explicitly in CustomButton now

claucambra added 21 commits May 30, 2023 10:00
Signed-off-by: Claudio Cambra <[email protected]>
@mgallien mgallien force-pushed the bugfix/fix-activities branch from 0acc015 to 4c096a8 Compare May 30, 2023 08:00
@mgallien mgallien enabled auto-merge May 30, 2023 08:00
@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-5715-4c096a885c0c1966c7a12a230c667a8c516068bd-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@mgallien mgallien merged commit 34792e0 into master May 30, 2023
@mgallien mgallien deleted the bugfix/fix-activities branch May 30, 2023 08:39
@mgallien mgallien added this to the 3.10.0 milestone Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants