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(core): Do not invert app menu text color #38776

Merged
merged 2 commits into from Jun 19, 2023

Conversation

Hephi2
Copy link
Contributor

@Hephi2 Hephi2 commented Jun 12, 2023

Summary

Instead of inverting the color of the whole app entry in the menubar when using a light background, only the color of the app icon is inverted. The invertion of the label color already happens here:

'--color-primary-element-text' => $this->util->invertTextColor($colorPrimaryElement) ? '#000000' : '#ffffff',

##Screenshots
Before:
image

After:
image

Tested on Chrome, Edge, Firefox and Opera Browser

@Hephi2 Hephi2 self-assigned this Jun 12, 2023
@Hephi2 Hephi2 added bug 3. to review Waiting for reviews labels Jun 12, 2023
@Hephi2 Hephi2 requested review from szaimen and Pytal and removed request for szaimen June 12, 2023 16:25
@szaimen szaimen requested review from a team and susnux and removed request for a team June 12, 2023 16:27
@szaimen szaimen added this to the Nextcloud 28 milestone Jun 12, 2023
@szaimen

This comment was marked as resolved.

@szaimen szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jun 12, 2023
@Hephi2 Hephi2 force-pushed the fix/app-menu-do-not-invert-text branch from 36986e3 to bc98d2c Compare June 13, 2023 09:11
@Hephi2 Hephi2 requested a review from szaimen June 13, 2023 10:20
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

Seems to fix the linked problem in my testing

@szaimen szaimen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 13, 2023
@mejo- mejo- added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 13, 2023
@Hephi2 Hephi2 force-pushed the fix/app-menu-do-not-invert-text branch from 2388f91 to d88bea8 Compare June 13, 2023 12:48
@szaimen
Copy link
Contributor

szaimen commented Jun 13, 2023

/compile /

@szaimen szaimen enabled auto-merge June 13, 2023 14:08
@mejo- mejo- disabled auto-merge June 13, 2023 14:09
@mejo-
Copy link
Member

mejo- commented Jun 13, 2023

The cypress tests need to be fixed first, I think @Hephi2 will do so later.

@Hephi2 Hephi2 force-pushed the fix/app-menu-do-not-invert-text branch 2 times, most recently from 6db2603 to 3244f95 Compare June 14, 2023 09:42
@Hephi2 Hephi2 force-pushed the fix/app-menu-do-not-invert-text branch from 3244f95 to 85db409 Compare June 14, 2023 11:41
@Hephi2
Copy link
Contributor Author

Hephi2 commented Jun 14, 2023

/compile

@Hephi2 Hephi2 enabled auto-merge June 14, 2023 11:50
@Hephi2 Hephi2 force-pushed the fix/app-menu-do-not-invert-text branch from 1632803 to ce9c17a Compare June 14, 2023 13:19
@mejo- mejo- force-pushed the fix/app-menu-do-not-invert-text branch from ce9c17a to 8e9a000 Compare June 14, 2023 13:44
@Hephi2
Copy link
Contributor Author

Hephi2 commented Jun 14, 2023

/compile

@susnux susnux force-pushed the fix/app-menu-do-not-invert-text branch from 8e9a000 to c8de6c4 Compare June 14, 2023 15:09
@susnux
Copy link
Contributor

susnux commented Jun 14, 2023

/compile

It is most of the time faster to compile and commit it locally :)

@mejo-
Copy link
Member

mejo- commented Jun 14, 2023

It is most of the time faster to compile and commit it locally :)

Thanks @susnux! But for the sake of easier backports its still better to keep source changes and compiled asset changes in separate commits, no?

@mejo-
Copy link
Member

mejo- commented Jun 14, 2023

I don't understand why the Cypress Tests fail here 🤔

@mejo- mejo- force-pushed the fix/app-menu-do-not-invert-text branch from c8de6c4 to 6b00ace Compare June 14, 2023 20:29
@juliushaertl
Copy link
Member

Restarted.

@susnux
Copy link
Contributor

susnux commented Jun 15, 2023

Locally it works. Cypress seems currently broken on CI (server master) (see all other CI runs)

@susnux susnux force-pushed the fix/app-menu-do-not-invert-text branch from b964a48 to 165d089 Compare June 15, 2023 14:18
@mejo- mejo- force-pushed the fix/app-menu-do-not-invert-text branch from 165d089 to e9d0463 Compare June 19, 2023 07:27
susnux and others added 2 commits June 19, 2023 09:49
* Also fixes other cypress test
* Build assets

Signed-off-by: Philipp Hempel <Philipp.Hempel1@web.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Jonas <jonas@freesources.org>
@mejo- mejo- force-pushed the fix/app-menu-do-not-invert-text branch from e9d0463 to 0397e9d Compare June 19, 2023 07:52
@mejo-
Copy link
Member

mejo- commented Jun 19, 2023

I splitted the PR between source code changes and compiled JS assets and rebased. Cypress CI test issues seem to have solved themselves 🪄

@Hephi2 Hephi2 merged commit 59f63a7 into master Jun 19, 2023
38 checks passed
@Hephi2 Hephi2 deleted the fix/app-menu-do-not-invert-text branch June 19, 2023 09:09
@welcome
Copy link

welcome bot commented Jun 19, 2023

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@Hephi2
Copy link
Contributor Author

Hephi2 commented Jun 19, 2023

/backport 7f86198 to stable27

@backportbot-nextcloud
Copy link

The backport to stable27 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable27
git pull origin stable27

# Create the new backport branch
git checkout -b fix/foo-stable27

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable27

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish backport-request bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Inverted app label color for light background color
5 participants