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

[5.2] atum light mode - Fix modal window link color #44176

Merged
merged 14 commits into from
Nov 7, 2024

Conversation

RickR2H
Copy link
Member

@RickR2H RickR2H commented Oct 1, 2024

Pull Request for Issue # .

Summary of Changes

With the browser in dark modus, In the backend (Atum template) the links have the wrong color when the template is toggled to the light modus. The reason is that in the component.php only the dark color scheme colors are loaded if the browser is is dark mode. My fix will make sure that the correct colors are used, depending or the color scheme selected.

In dark modus the default hover color in the modal is defaulted to white, which is not correct. This PR fixes also the hover color which is now set to a lighter shade of the default link color.

Testing Instructions

  1. Set the browser you are using in dark mode.
  2. In the backend from Joomla select from the User Menu in the top right the link: Light/Dark Mode, and switch the template to light modus.
  3. Open a modal. This can be done by opening an article and selecting the Created By button in the Publishing tab.
    image
  4. The link color has a light blue color which is not accessible.
  5. Switch Joomla to Dark mode.
  6. The link hover color is white.

Apply the PR

  1. Switch to light modus.
  2. The link color is now the correct color and readable.
  3. Switch to dark mode.
  4. The link hover color is now light blue.

Actual result BEFORE applying this Pull Request

The link is too light which is a accessibility issue.

image

In dark mode the hover color white

image

Expected result AFTER applying this Pull Request

The link has the correct color.

image

In dark mode the hover color light blue

image

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@ChristineWk
Copy link

I have tested this item ✅ successfully on 6150056


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44176.

@brianteeman
Copy link
Contributor

Please also check #43708

@richard67
Copy link
Member

@RickR2H Adding the // phpcs:disable PSR1.Files.SideEffects hasn't helped. I think the issue is not the defined('_JEXEC') or die; but the function definition here: https://github.com/joomla/joomla-cms/pull/44176/files#diff-3538b524e6cf4f442f5ca0631475c0ef2b867edbcb23a24b5deb2248c428e252R35-R42 . Maybe it needs to wrap that with // phpcs:disable PSR1.Files.SideEffects and // phpcs:enable PSR1.Files.SideEffects?

@fgsw
Copy link

fgsw commented Oct 2, 2024

@RickR2H Can you add [5.2] to the title so user don't have to open the Pull Request to know which branche it is for?

@richard67
Copy link
Member

I‘ve restored the previous human test result as the changes after that were only hints for PHPCS.

@richard67
Copy link
Member

@RickR2H Please don’t do unnecessary branch Updates if there are no conflicts as the branch update invalidates the human test counter and I cannot watch your PR all day long to restore the tests.

@RickR2H RickR2H changed the title atum light mode - Fix modal window link color [5.2] atum light mode - Fix modal window link color Oct 2, 2024
@RickR2H
Copy link
Member Author

RickR2H commented Oct 2, 2024

@dgrammatiko @richard67 I can't get drone to behave... Any tips?

@richard67
Copy link
Member

@dgrammatiko @richard67 I can't get drone to behave... Any tips?

With the previous version with my proposal it was ok. So I suggest to wrap the method again into the // phpcs:disable PSR1.Files.SideEffects and // phpcs:enable PSR1.Files.SideEffects.

@fgsw
Copy link

fgsw commented Oct 6, 2024

I have tested this item ✅ successfully on c21bf32


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44176.

@ChristineWk
Copy link

I have tested this item ✅ successfully on c21bf32


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44176.

@richard67
Copy link
Member

@RickR2H Could you check my 2 suggestions for how to get rid of these PHPCS hints? It would not need new human tests if you apply them.

@C-Lodder
Copy link
Member

C-Lodder commented Oct 8, 2024

Why use PHP to manipulate CSS colours, when this can be acheived in native CSS with 1 line of code?

@Quy Quy added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Oct 8, 2024
@dgrammatiko
Copy link
Contributor

Why use PHP to manipulate CSS colours, when this can be acheived in native CSS with 1 line of code?

Because BS is using a css var of type number, number, number :
https://github.com/twbs/bootstrap/blob/fee9dc2438736a02405412e10ea8445dd9aeac1e/dist/css/bootstrap.css#L40-L47

@richard67
Copy link
Member

I've restored the previous human test results in the issue tracker as the commits which have invalidated the test count were only related to making PHPCS happy. In addition I have tested that the PR still works with the last change to an anonymous function.

@richard67
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44176.

@richard67 richard67 removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Oct 8, 2024
@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 8, 2024
@C-Lodder
Copy link
Member

C-Lodder commented Oct 8, 2024

@RickR2H
Copy link
Member Author

RickR2H commented Oct 8, 2024

@dgrammatiko https://jsfiddle.net/e5arm7db ?

Could be a nice option to replace the current functions. More clean! For now the the colors nor loading issues is fixed. This is a nice option to add with a new PR. Some logic is used in the index.php

@dgrammatiko
Copy link
Contributor

July 2024 might be too soon:
https://caniuse.com/css-relative-colors
Screenshot 2024-10-08 at 5 13 54 PM

Anyways if the maintainers are ok with it, go for it, it's not my call. I just pointed that the BS requires a weird value and that the relative color from is a new addition 🤷‍♂️

@richard67
Copy link
Member

@RickR2H To be honest, to me the hover colour seems to be inconsistent now when comparing dark mode and light mode.

In light mode it is still black with your PR, so when changing to dark mode I would expect it to be white.

Or vice versa, when coming from dark mode where it is light blue with your PR, and changing to light mode, I would expect it to be some other kind of blue but not black.

But maybe that's just me.

@RickR2H
Copy link
Member Author

RickR2H commented Oct 15, 2024

I guess there will be a better solution. I have no clue why the values have to calculated. We can make then fixed variables which is a better option. Probably is has to do with the backend colors...

@dgrammatiko
Copy link
Contributor

I have no clue why the values have to calculated.

The blame here goes to Bootstrap, they store the rgb values as a coma separated val: --bs-dark-rgb: 33, 37, 41; https://github.com/twbs/bootstrap/blob/fee9dc2438736a02405412e10ea8445dd9aeac1e/dist/css/bootstrap.css#L40-L47

Probably is has to do with the backend colors...

Nope, search the bs css and you'll see how these values are used

I guess there will be a better solution.

What Charlie suggested but the browser support is only few months...

@pe7er pe7er self-assigned this Oct 30, 2024
@wilsonge
Copy link
Contributor

This is consistent with index.php so I'd merge this if it fixes the bug - then at least if we ever change to @C-Lodder 's way in the future (j6 or whatever) then we can do it consistently. Only thing I'd suggest is that we have this the error_login.php and error_full.php too so we have the same set of css variables in all 3 files.

@pe7er pe7er enabled auto-merge (squash) November 7, 2024 08:02
@pe7er pe7er merged commit 2f401da into joomla:5.2-dev Nov 7, 2024
3 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 7, 2024
@pe7er
Copy link
Contributor

pe7er commented Nov 7, 2024

Thanks @RickR2H !

@dgrammatiko
Copy link
Contributor

Only thing I'd suggest is that we have this the error_login.php and error_full.php too so we have the same set of css variables in all 3 files.

@RickR2H please do another PR with the same code covering the other 2 files @wilsonge mentioned. Thanks

@Hackwar Hackwar added this to the Joomla! 5.2.1 milestone Nov 7, 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.