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

Add contrast to the text/icon of buttons if the server defined color is light. #4641

Merged
merged 1 commit into from
Jun 21, 2022
Merged

Add contrast to the text/icon of buttons if the server defined color is light. #4641

merged 1 commit into from
Jun 21, 2022

Conversation

camilasan
Copy link
Member

Before:

before-primary-button
before

After:

after-primary-button
after

@camilasan
Copy link
Member Author

/backport to stable-3.5

@codecov
Copy link

codecov bot commented Jun 14, 2022 •
edited
Loading

Codecov Report

Merging #4641 (dca696f) into master (19bb164) will not change coverage.
The diff coverage is n/a.

❗ Current head dca696f differs from pull request most recent head fc2408c. Consider uploading reports for the commit fc2408c to get more accurate results

@@           Coverage Diff           @@
##           master    #4641   +/-   ##
=======================================
  Coverage   56.43%   56.43%           
=======================================
  Files         138      138           
  Lines       17070    17070           
=======================================
  Hits         9634     9634           
  Misses       7436     7436
Impacted Files Coverage Δ
src/libsync/propagateupload.cpp 76.41% <0.00%> (-0.66%) ⬇️
src/libsync/discovery.cpp 84.86% <0.00%> (+0.29%) ⬆️

@camilasan
Copy link
Member Author

/rebase

nimishavijay
nimishavijay approved these changes Jun 15, 2022
View reviewed changes
Copy link
Member

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

Looks great!

jancborchardt
jancborchardt approved these changes Jun 16, 2022
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.

Better! :) Btw a bit unrelated, but the "Reply" button does not need a title which shows on hover – it’s bad for accessibility, as the label is already "Reply", it doesn’t need to duplicate that.

claucambra
claucambra reviewed Jun 16, 2022
View reviewed changes
Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

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

Small comment on my part and agree with @mgallien, but besides that it looks good :)

src/gui/tray/ActivityItem.qml Outdated Show resolved Hide resolved
@camilasan camilasan force-pushed the bugfix/userheadercolor-contrast branch 3 times, most recently from dca696f to c7030cd Compare June 16, 2022 21:29
@camilasan
Copy link
Member Author

/rebase

@camilasan camilasan force-pushed the bugfix/userheadercolor-contrast branch from c7030cd to e5d9522 Compare June 20, 2022 12:02
@camilasan
Copy link
Member Author

/rebase

Add contrast to the text/icon of buttons if the server defined color …
fc2408c 
…is light.

Signed-off-by: Camila <hello@camila.codes>
@camilasan camilasan force-pushed the bugfix/userheadercolor-contrast branch from e5d9522 to fc2408c Compare June 21, 2022 09:49
@camilasan camilasan mentioned this pull request Jun 21, 2022
Merged
@sonarcloud
Copy link

sonarcloud bot commented Jun 21, 2022

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

@nextcloud-desktop-bot
Copy link

AppImage file: Nextcloud-PR-4641-fc2408cf415bb496bd7f286744e2e8ae8531a5bd-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.

mgallien
mgallien approved these changes Jun 21, 2022
View reviewed changes
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.

nice

@mgallien mgallien merged commit 3a53a1c into master Jun 21, 2022
@mgallien mgallien deleted the bugfix/userheadercolor-contrast branch June 21, 2022 11:11
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jun 21, 2022
Merged
@mgallien mgallien added this to the 3.6.0 milestone Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@claucambra claucambra claucambra approved these changes

@nimishavijay nimishavijay nimishavijay approved these changes

@jancborchardt jancborchardt jancborchardt approved these changes

@mgallien mgallien mgallien approved these changes

@allexzander allexzander Awaiting requested review from allexzander

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.6.0
Development

Successfully merging this pull request may close these issues.
6 participants
@camilasan @nextcloud-desktop-bot @mgallien @jancborchardt @nimishavijay @claucambra