Skip to content

[5.3] adding scss variables.#45231

Merged
rdeutz merged 2 commits intojoomla:5.3-devfrom
hiteshm0:new_branch
Jul 31, 2025
Merged

[5.3] adding scss variables.#45231
rdeutz merged 2 commits intojoomla:5.3-devfrom
hiteshm0:new_branch

Conversation

@hiteshm0
Copy link
Contributor

@hiteshm0 hiteshm0 commented Mar 27, 2025

Pull Request for Issue #45224

Summary of Changes

Adding missing scss variables :
In dark mode:
btn-secondary-color-hvr

In light mode:
btn-secondary-color-hvr
btn-secondary-bg-hvr

Fallback has also been added in buttons.scss for the hover effect of secondary btn classs

Testing Instructions

Refer issue #45224

Hover over any button with class btn-secondary ( The Toggle Editor button while creating an article is one such button ) and check that the btn-secondary-color-hvr and btn-secondary-bg-hvr is defined and is being applied via the DevTools ( previously this wasn't the case )

Actual result BEFORE applying this Pull Request

When you hover over a button with class btn-secondary and go to inspect element it says that btn-secondary-color-hvr,
btn-secondary-bg-hvr is not defined.

Expected result AFTER applying this Pull Request

The above issue should be fixed and btn-secondary-color-hvr, btn-secondary-bg-hvr should apply to the buttons

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

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.2-dev labels Mar 27, 2025
@brianteeman
Copy link
Contributor

why do you need the fallback?

@hiteshm0
Copy link
Contributor Author

hiteshm0 commented Mar 27, 2025

If in any case these variables aren't defined, but i wouldn't be able to name what those cases are, thought i would add it just as a safety measure. I shouldve been more mindful of changing the code , would be happy to revert it back

@brianteeman
Copy link
Contributor

I am not great with scss but it would appear to me that if the first variable wasnt present then the second one wouldnt be either as they are in the same file

@hiteshm0
Copy link
Contributor Author

@brianteeman yes, you're right the advantage of having this fallback is if, in the future the variables are moved or something similar to that. Should i remove the fallback ?

@brianteeman
Copy link
Contributor

@brianteeman yes, you're right the advantage of having this fallback is if, in the future the variables are moved or something similar to that. Should i remove the fallback ?

I leave that decision to someone with better scss knowledge than me

@hiteshm0 hiteshm0 marked this pull request as ready for review March 27, 2025 11:18
@ghost
Copy link

ghost commented Mar 27, 2025

I assume that this needs to be rebased to a different branch as there will be no further 5.2 releases.

Originally posted by @brianteeman in #45229 (comment)

@hiteshm0
Copy link
Contributor Author

@fgsw should i rebase it to 5.3-dev ?

@ghost
Copy link

ghost commented Mar 27, 2025

@hiteshm0 Please read the comment by @brianteeman.

@hiteshm0
Copy link
Contributor Author

@fgsw got it , ill wait for further intimation on what to do.

@rdeutz
Copy link
Contributor

rdeutz commented Mar 28, 2025

You can rebase to 5.3 and we can try to merge it into 5.3.1

@hiteshm0 hiteshm0 changed the base branch from 5.2-dev to 5.3-dev March 28, 2025 13:07
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-5.3-dev labels Mar 28, 2025
@hiteshm0 hiteshm0 changed the title adding scss variables. [5.3] adding scss variables. Mar 28, 2025
@joomla-cms-bot joomla-cms-bot removed the Language Change This is for Translators label Mar 28, 2025
@hiteshm0 hiteshm0 marked this pull request as draft March 29, 2025 03:43
@hiteshm0 hiteshm0 marked this pull request as ready for review March 29, 2025 16:27
@hiteshm0
Copy link
Contributor Author

@rdeutz i've rebased it to 5.3

@ceford
Copy link
Contributor

ceford commented May 27, 2025

I don't quite understand the testing instructions. With the patch applied I see the change in appearance of secondary buttons in the atum template but I do not see btn-secondary-color-hvr, btn-secondary-bg-hvr in the Inspector. What I see is

.btn-secondary {
    --btn-color: #fff;
    --btn-bg: #495057;
    --btn-border-color: #495057;
    --btn-hover-color: #fff;
    --btn-hover-bg: rgb(62.05, 68, 73.95);
    --btn-hover-border-color: rgb(58.4, 64, 69.6);
    --btn-focus-shadow-rgb: 100, 106, 112;
    --btn-active-color: #fff;
    --btn-active-bg: rgb(58.4, 64, 69.6);
    --btn-active-border-color: rgb(54.75, 60, 65.25);
    --btn-active-shadow: inset 0 3px 5px rgba(0, 0, 0, 0.125);
    --btn-disabled-color: #fff;
    --btn-disabled-bg: #495057;
    --btn-disabled-border-color: #495057;
}

And it does not change when I add/remove the patch and do npm run build:css

Is the change in button appearance sufficient for this test?


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

@hiteshm0
Copy link
Contributor Author

@ceford hello, I apologise if the testing instructions were not proper.
Can you please check .btn-secondary:hover in the inspect tool.
And is the change in appearance desirable ??

@ceford
Copy link
Contributor

ceford commented May 27, 2025

@ceford hello, I apologise if the testing instructions were not proper. Can you please check .btn-secondary:hover in the inspect tool. And is the change in appearance desirable ??

Here is a screenshot of what I see in Firefox Developer Tools. The Toggle Editor button is selected and you can see the compiled .btn-secondary styles. I do not see any btn-secondary-color-hvr and btn-secondary-bg-hvr attributes. Am I using the wrong Inspect tool?

image

Having explored some more I found the -hvr styles much further down the styles list than I have been before. And absent unless the patch is applied. So the PR works. The change of colour on hover seems rather small, barely noticeable. I wonder if there are any guidelines on that.

@ceford
Copy link
Contributor

ceford commented May 27, 2025

I have tested this item ✅ successfully on 0580b7a

The hover contrast in Bootstrap 5.3 buttons is barely evident too so I guess this PR is fine.


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

@hiteshm0
Copy link
Contributor Author

hiteshm0 commented May 28, 2025

thank you 😄 👍@ceford

@RickR2H
Copy link
Member

RickR2H commented Jul 31, 2025

I have tested this item ✅ successfully on 0580b7a

If custom properties are used in the build CSS, they should al least have a value. This is the correct implementation with fallback.


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

@RickR2H
Copy link
Member

RickR2H commented Jul 31, 2025

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 31, 2025
@rdeutz rdeutz added bug and removed PR-5.2-dev labels Jul 31, 2025
@rdeutz rdeutz merged commit 0e88af4 into joomla:5.3-dev Jul 31, 2025
39 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 31, 2025
@rdeutz
Copy link
Contributor

rdeutz commented Jul 31, 2025

Thank you all.

@hiteshm0
Copy link
Contributor Author

thank you everybody, this is my first OS contribution.

@QuyTon QuyTon added this to the Joomla! 5.3.3 milestone Aug 3, 2025
@hiteshm0 hiteshm0 deleted the new_branch branch January 9, 2026 12:35
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.

8 participants