Skip to content

Comments

[4.3] Button text color issue#40435

Merged
obuisard merged 6 commits intojoomla:4.3-devfrom
sandewt:4.3-dev
Jul 2, 2023
Merged

[4.3] Button text color issue#40435
obuisard merged 6 commits intojoomla:4.3-devfrom
sandewt:4.3-dev

Conversation

@sandewt
Copy link
Contributor

@sandewt sandewt commented Apr 20, 2023

Pull Request for Issue #40306

Summary of Changes

Changes some code in _global.scss file

Testing Instructions

Actual result BEFORE applying this Pull Request

Text NOT continues to be read.

Expected result AFTER applying this Pull Request

Text continues to be read.

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

Thanks @joomlaphp and @dgrammatiko

Button text color issue
@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.3-dev labels Apr 20, 2023
@richard67 richard67 added the bug label Apr 20, 2023
@sandewt
Copy link
Contributor Author

sandewt commented Apr 21, 2023

@dgrammatiko for clarity

I used this code:

  &:not(.btn):hover,
  &:not(.btn):focus {
    color: var(--cassiopeia-color-hover);
  }

Instead of the code from your comment, see #40306 (comment)
Because this last one caused an npm error.

@N6REJ
Copy link
Contributor

N6REJ commented Apr 21, 2023

I have tested this item ✅ successfully on 3399b30


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

@sandewt
Copy link
Contributor Author

sandewt commented Apr 22, 2023

Drone: Build is failing
Does this have to do with this pr or just ignore this message?

@sandewt
Copy link
Contributor Author

sandewt commented Apr 22, 2023

@joomlaphp

I made the changes in the templates/site/cassiopeia/scss/blocks/_global.scss file. Do I need to make changes to another file as well?
Browser cookies + Joomla cookies cleared.

Please, give your comment here #40435

Here you will find information about testing: https://docs.joomla.org/Testing_Joomla!_patches

@joomla joomla deleted a comment from N6REJ Apr 22, 2023
@sandewt
Copy link
Contributor Author

sandewt commented May 28, 2023

I would benefit from testing this pr, thanks in advance.

@joomlaphp

@joomlaphp
Copy link

joomlaphp commented May 30, 2023

I would benefit from testing this pr, thanks in advance.

@joomlaphp

Hi @sandewt ,

No problem as there is no link:: <button type="button" class="btn btn-primary">Primary</button>

The problem persists after the link is given. All cookies etc. I cleaned: <a class="btn btn-primary" href="#" role="button">Link</a>

I'm good at CSS and I can fix it with user.css but it's so annoying :)
Regards

@sandewt
Copy link
Contributor Author

sandewt commented May 30, 2023

The problem persists after the link is given. All cookies etc. I cleaned:

Did you run npm ci after installing the patch ?

[EDIT @joomlaphp does the thumb mean that you run npm ci ? Yes or no ?]

@chmst
Copy link
Contributor

chmst commented Jun 18, 2023

I have tested this item ✅ successfully on 26b3009


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

@joomla-cms-bot joomla-cms-bot changed the title [4.3] Button text color issue [4.3] Button text color issue Jun 18, 2023
@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 18, 2023
@N6REJ
Copy link
Contributor

N6REJ commented Jun 18, 2023

RTC

@obuisard obuisard modified the milestone: Joomla! 4.3.3 Jun 19, 2023
@obuisard obuisard added this to the Joomla! 4.3.3 milestone Jul 1, 2023
@obuisard obuisard merged commit 3a44db8 into joomla:4.3-dev Jul 2, 2023
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 2, 2023
@obuisard
Copy link
Contributor

obuisard commented Jul 2, 2023

Thank you for the PR @sandewt :-)

@sandewt
Copy link
Contributor Author

sandewt commented Jul 2, 2023

Thanks all.

@brianteeman
Copy link
Contributor

This needs to be reverted as it caused #41153 and #41172

@sandewt
Copy link
Contributor Author

sandewt commented Jul 19, 2023

This needs to be reverted as it caused #41153 and #41172

That's just the question. These both issues (#41153 and #41172) are easy to solve with some extra css code.

@sandewt
Copy link
Contributor Author

sandewt commented Jul 22, 2023

This needs to be reverted as it caused #41153 and #41172

I'm looking for a solution, my finding is the following:

The :not() CSS pseudo-class represents elements that do not match a list of selectors. See: https://developer.mozilla.org/en-US/docs/Web/CSS/:not

In this case these are the buttons (.btn) that are excluded, what the desired behavior is. See the code below.

a:not(.btn):hover, a:not(.btn):focus {
  color: var(--cassiopeia-color-hover);
}

So it's strange (irrelevant) that elements in the footer that have nothing to do with a button, act on it.

This justifies not undoing this pr.

@brianteeman what is your opinion ? Do you have another solution for this pr ?

@brianteeman
Copy link
Contributor

you misunderstood the css

a:not(.btn):hover, a:not(.btn):focus {
  color: var(--cassiopeia-color-hover);
}

in english that says

when you hover over any link that is not a button then set the color to blue

So the color of the links in the footer and the back-to-top (which is a link not a button) change to blue when you hover.

@brianteeman
Copy link
Contributor

They did not before because they were set by the css

.footer a {
    color: currentColor;
}

but the change made in this PR override that

@sandewt
Copy link
Contributor Author

sandewt commented Jul 23, 2023

but the change made in this PR override that

That's why I want to extend the css, because I have no other solution for this PR.

.footer a, .footer a:focus, .footer a:hover {
  color: currentColor;
}

@obuisard
Copy link
Contributor

obuisard commented Aug 1, 2023

Have you tried to add?

.footer a:not(.btn):hover, .footer a:not(.btn):focus {
  color: currentColor;
}

@sandewt
Copy link
Contributor Author

sandewt commented Aug 2, 2023

Have you tried to add?

Yes, I have replaced this pr with your code proposal.
And it works. 👍

[EDIT It doesn't work on any other website 👎 ]

@sandewt
Copy link
Contributor Author

sandewt commented Aug 3, 2023

Have you tried to add?

If you meant this suggestion only for the footer, we'd better have the discussion here: #41172

@obuisard
Copy link
Contributor

Have you tried to add?

If you meant this suggestion only for the footer, we'd better have the discussion here: #41172

No, I meant to add the specific code for the footer to complement this PR and fix all related issues at once.

GeraintEdwards pushed a commit to GeraintEdwards/joomla-cms that referenced this pull request Aug 14, 2023
@jackal-kr
Copy link

This needs to be reverted as it caused #41153 and #41172

That's just the question. These both issues (#41153 and #41172) are easy to solve with some extra css code.

Hello, i have those two also after upgrade to 4.3.3 - can you please help me with the workaround in user.css? I am new to css and joomla so that fixing myself will take some time... thanks a lot!

also any idea, when these two will be fixed? in 4.4.4?,,,

@sandewt
Copy link
Contributor Author

sandewt commented Aug 16, 2023

Have you tried to add?

Yes, but not with the desired result.

No, I meant to add the specific code for the footer to complement this PR and fix all related issues at once.

Sorry @obuisard , it's not quite clear to me what you mean. If you would like to make the PR in question. I'll test.

@obuisard
Copy link
Contributor

obuisard commented Aug 16, 2023

also any idea, when these two will be fixed? in 4.3.4?

No, it won't, the version is already in RC phase, any fix will have to go to 4.3.5.

@obuisard
Copy link
Contributor

Sorry @obuisard , it's not quite clear to me what you mean. If you would like to make the PR in question. I'll test.

Ok, I'll give it a try (probably not today) and see if can come up with a fix that takes care of everything.

@obuisard
Copy link
Contributor

obuisard commented Aug 17, 2023

I was starting to look for a solution to fix this, but, to be honest, I cannot even replicate the initial issue stated in #40306.

image

My fix, in this instance, would be to revert this PR, as suggested by Brian @brianteeman early on.

Ugur @joomlaphp, before we do such a thing, could you share the URL of the site where you had the issue in the first place? I don't want to dismiss any issue and I need to see if your initial issue was not related to something else.

@sandewt
Copy link
Contributor Author

sandewt commented Aug 26, 2023

This needs to be reverted as it caused #41153 and #41172

I think this is the overall solution:

Change the code of this pr

a:not(.btn):hover, a:not(.btn):focus {
  color: var(--cassiopeia-color-hover);
}

in

a:not([class]):hover, a:not([class]):focus {
  color: currentColor;
}

We (@RickR2H and I) have tested this code in the PBF.

[EDIT I have tested in 4.4.0-alpha5-dev + 4.3.5-dev]

@sandewt
Copy link
Contributor Author

sandewt commented Aug 27, 2023

I was starting to look for a solution to fix this, but, to be honest, I cannot even replicate the initial issue stated in #40306.

@obuisard I can reproduce this issue in J4.3.5-dev, see:

Schermafbeelding 2023-08-27 105721

Method:
Create a custom module in the banner position, with the following button code:
<p><a class="btn btn-success" href="#!">Get started</a></p>

I changed the code to the original one:
\joomla\build\media_source\templates\site\cassiopeia\scss\blocks_global.scss

Line 63:
&:hover,
  &:focus {
    color: var(--cassiopeia-color-hover);
  }

Run: npm ci

@sandewt
Copy link
Contributor Author

sandewt commented Sep 13, 2023

I think this is the overall solution:

I am waiting for response(s), if ok then I will make a new PR.

@obuisard
Copy link
Contributor

I am waiting for response(s), if ok then I will make a new PR.

Yes, if you make a new PR to fix the issue, then we can have people test it. Thanks @sandewt !

@sandewt
Copy link
Contributor Author

sandewt commented Sep 14, 2023

Please test PR #41749

@sandewt sandewt deleted the 4.3-dev branch August 19, 2024 13:50
@sandewt sandewt restored the 4.3-dev branch August 19, 2024 13:51
@sandewt sandewt deleted the 4.3-dev branch August 19, 2024 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants