joomla-cms icon indicating copy to clipboard operation
joomla-cms copied to clipboard

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

Open RickR2H opened this issue 1 year ago • 21 comments

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

RickR2H avatar Oct 01 '24 12:10 RickR2H

I have tested this item :white_check_mark: successfully on 61500560b6faf9d79d8c2e400cdaabc0363f118f


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

ChristineWk avatar Oct 01 '24 14:10 ChristineWk

Please also check #43708

brianteeman avatar Oct 01 '24 15:10 brianteeman

@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?

richard67 avatar Oct 01 '24 17:10 richard67

@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?

ghost avatar Oct 02 '24 04:10 ghost

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

richard67 avatar Oct 02 '24 04:10 richard67

@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.

richard67 avatar Oct 02 '24 04:10 richard67

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

RickR2H avatar Oct 02 '24 09:10 RickR2H

@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.

richard67 avatar Oct 02 '24 13:10 richard67

I have tested this item :white_check_mark: successfully on c21bf325d7b6ce7e8d0f8dcaa573402b61bbebea


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

ghost avatar Oct 06 '24 09:10 ghost

I have tested this item :white_check_mark: successfully on c21bf325d7b6ce7e8d0f8dcaa573402b61bbebea


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

ChristineWk avatar Oct 07 '24 15:10 ChristineWk

@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.

richard67 avatar Oct 07 '24 16:10 richard67

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

C-Lodder avatar Oct 08 '24 08:10 C-Lodder

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

dgrammatiko avatar Oct 08 '24 13:10 dgrammatiko

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 avatar Oct 08 '24 13:10 richard67

RTC


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

richard67 avatar Oct 08 '24 13:10 richard67

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

C-Lodder avatar Oct 08 '24 14:10 C-Lodder

@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

RickR2H avatar Oct 08 '24 14:10 RickR2H

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 🤷‍♂️

dgrammatiko avatar Oct 08 '24 14:10 dgrammatiko

@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.

richard67 avatar Oct 13 '24 11:10 richard67

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...

RickR2H avatar Oct 15 '24 12:10 RickR2H

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...

dgrammatiko avatar Oct 15 '24 12:10 dgrammatiko

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.

wilsonge avatar Oct 30 '24 18:10 wilsonge

Thanks @RickR2H !

pe7er avatar Nov 07 '24 08:11 pe7er

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

dgrammatiko avatar Nov 07 '24 08:11 dgrammatiko