@ravisaxena23 Sorry, I can review only code style. That looks good to me right now. If the scss is right I can't really say, I'm not an scss expert. Maybe @brianteeman can review, or @C-Lodder ? And by the way, no need to call me "sir". Just say "Richard" ;-)

Sure sir 😜

To be honest, I'm in doubt if the change of the CSS class from "mm-collapsed" to "collapsed" is right. As far as I know we use metismenu in J4 backend, and the "mm-" class prefix belongs to that. @brianteeman Could you check this?

Update few minutes later: Just checked the buttons in the html inspector of browser developer tools: They have indeed class "collapsed" and not "mm-collapsed". So either this PR is correct, or our code is wrong at some other place and the buttons should have class "mm-collapsed".

Will need checking carefully as we do indeed use mm- for Metis menu

@ravisaxena23 Meanwhile I've tested this Pull Request (PR), and it works.

Also I know meanwhile that the change of class "mm-collapsed" to "collapsed" in the scss file is correct. The metismenu js (or any other js?) adds and removes the "collapsed" class (and not "mm-collapsed") when toggling the button. I could see that when inspecting html with the developer tools of my browser. Update: Not sure anymore about this, see my next comment below.

So this PR fixes the error that the lower button always shows the cross icon and not burger or cross depending on if collapsed or expanded. The gears button continues to work as expected, too.

And an alert can be closed now, too.

So this PR does what it promises, and it seems not to break other functionality ...

BUT: If the screen is really small enough that the button to close an alert is hidden by the top button (gears icon if collapsed) whithout this PR, then with this PR some text of the alert message is hidden by the button:

Not sure if this is acceptable.

Maybe instead of moving the gears button down it would be better to change z-index of the alert so it comes in front of the gears button, or change the z-index of the gears button so it comes behind the alert?

@brianteeman What do you think?

Update: It happens really only on very very very small screens that the gears button hides the close button of the alert without this PR or hides a part of the message with this PR. So maybe that part of the PR should be reverted? What remains then is the correct change of the class name in the scss so the icon of the burger button changes when collapsing and expanding.

Hmm, the metismenu js seems to use "mm-collapsed" at least here:

.

And when searching our sources for "mm-collapsed" I find:

./media/mod_menu/js/admin-menu.js:106:      sidebar.querySelector('.mm-collapse').classList.remove('mm-collapsed');
./media/mod_menu/js/admin-menu.es6.js:106:      sidebar.querySelector('.mm-collapse').classList.remove('mm-collapsed');
./media/vendor/metismenujs/js/metismenujs.js:50:        COLLAPSED: "mm-collapsed",

Am confused now why the icon toggling of the burger button doesn't work without this PR and works with it, while for the gears button nothing changes, i.e. it works with and without this PR.

There must be something else wrong in our code, but I haven't found it yet and have to finish for today, is late here.

check the code that I added for the gear button. I probably broke it all there

@brianteeman You remember which PR it was, or key words I could search? Or where code to check?

I think it was #26513

@brianteeman @richard67 what if we decrease the size of gear button and move it a little down. Therefore it won't hide alert and modals text with full functionality can we do that should I try? @brianteeman @richard67 sir i had a question how yo test PR without accepting how anyone can do that?

sir i had a question how yo test PR without accepting how anyone can do that?

@ravisaxena23 Sorry, I don't understand what you mean with that.

@richard67 I'm Saying can we do it? as you told earlier something can be done with z index. i had created a new css for gear button now it can look like this. it will not harm burger button as it has its own css after my new code update it will look like this should I update code or revert the changes done in gear button?

@richard67 I was asking how to test PRs without accepting there changes.

@richard67 I'm Saying can we do it? as you told earlier something can be done with z index. i had created a new css for gear button now it can look like this. it will not harm burger button as it has its own css after my new code update it will look like this should I update code or revert the changes done in gear button?

The result look good, but I don't know how it is achieved when not seeing the code. Maybe you can paste it into a comment so we can see and tell our opinion? Or commit the changes to this PR and then we review it.

@richard67 I was asking how to test PRs without accepting there changes.

I'm still not sure if I understand. When a PR is made, we first look on it and the changes it makes, and if necessary we comment it. That's review and that's what we are doing now.

Then for testing it we apply the changes in our local Joomla testing environment. this can be done with different tools, e.g. tortoise git, or with the Joomla patchtester in some case.

Then if necessary we use npm to compile the scss files into css. And then we test the PR. I've done that yesterday, and I saw need for discussion so I did not give a bad test result due to the button overlapping the alert text.

After test we mark the test result in the Joomla CMS issue tracker https://issues.joomla.org/ . If the result is good, we mark it as success, and if the result is not as expected or the PR breaks something else, we mark it as not successful.

Then when a PR has at least 2 successful tests, we mark it with label "RTC", which means "ready to commit".

Peridocially our maintainers (I am not one, I am only a contributor like you are) check if there are PRs having RTC. They normally check it again with review, and if it looks good it is merged.

After merge the changes of the PR have been applied to the CMS code, and with the next release the changes will be available to all end users when they update their CMS.

Did you mean that with "accepting there changes", how a PR will go into the CMS finally? If not, please explain in other words or more precisely what you mean.

We have documentation available on https://docs.joomla.org for that (testing PRs), especially in 4.0 where you need composer and npm, but I'm at work now so don't have the time to search the links to particular docs. If you still have question let us know, then me or someone else who has time can later paste a few more links to documentation.

@richard67 @brianteeman after latest changes mobile view is updated and it don't hide any text or closing button please review

@richard67 I was asking how to test PRs without accepting there changes.

I'm still not sure if I understand. When a PR is made, we first look on it and the changes it makes, and if necessary we comment it. That's review and that's what we are doing now.

Then for testing it we apply the changes in our local Joomla testing environment. this can be done with different tools, e.g. tortoise git, or with the Joomla patchtester in some case.

Then if necessary we use npm to compile the scss files into css. And then we test the PR. I've done that yesterday, and I saw need for discussion so I did not give a bad test result due to the button overlapping the alert text.

After test we mark the test result in the Joomla CMS issue tracker https://issues.joomla.org/ . If the result is good, we mark it as success, and if the result is not as expected or the PR breaks something else, we mark it as not successful.

Then when a PR has at least 2 successful tests, we mark it with label "RTC", which means "ready to commit".

Peridocially our maintainers (I am not one, I am only a contributor like you are) check if there are PRs having RTC. They normally check it again with review, and if it looks good it is merged.

After merge the changes of the PR have been applied to the CMS code, and with the next release the changes will be available to all end users when they update their CMS.

Did you mean that with "accepting there changes", how a PR will go into the CMS finally? If not, please explain in other words or more precisely what you mean.

We have documentation available on https://docs.joomla.org for that (testing PRs), especially in 4.0 where you need composer and npm, but I'm at work now so don't have the time to search the links to particular docs. If you still have question let us know, then me or someone else who has time can later paste a few more links to documentation.

@richard67 thank you sir for explaining the whole process."my English is weak" you understood it and give me the answer it will help me a lot.

@richard67 I'm Saying can we do it? as you told earlier something can be done with z index. i had created a new css for gear button now it can look like this. it will not harm burger button as it has its own css after my new code update it will look like this should I update code or revert the changes done in gear button?

The result look good, but I don't know how it is achieved when not seeing the code. Maybe you can paste it into a comment so we can see and tell our opinion? Or commit the changes to this PR and then we review it.

I have commited boss please suggest me I just want to learn more and more ; )

I just want to learn more and more ; )

Then the first thing you should learn is that we don't have any "boss" here.

I just want to learn more and more ; )

Then the first thing you should learn is that we don't have any "boss" here.

okay please review Pr @richard67

@ravisaxena23 I am the wrong guy to review (s)css because I'm not an expert in it. Maybe @C-Lodder can have a look?

@ravisaxena23 I am the wrong guy to review (s)css because I'm not an expert in it. Maybe @C-Lodder can have a look?

okay

@ravisaxena23 But of course I want to help, so if someone who knows our scss better than me has made a review and then (maybe after some corrections due to the review) you need tests, I promise I will test your PR.

@richard67 thank you you are great help how can I contact you or shall I ask my doubts in PR i will always create a PR for your guidance 😜 haha

SCSS properties are in the wrong order but Hound should be picking up on this.
Not sure why Hound is so temprimental

Other than that, code looks fine

SCSS properties are in the wrong order but Hound should be picking up on this.
Not sure why Hound is so temprimental

What would be the right order?

@richard67 Based on the current linter (https://github.com/joomla/joomla-cms/blob/4.0-dev/scss-lint.yml):

.toggler-toolbar {
  position: fixed;
  top: 20px;
  right: 10px;
  bottom: 20px;
  z-index: 1030;
  display: block;
  width: 70px;
  height: 70px;
  background: var(--atum-link-color);
  border: 8px solid var(--atum-bg-light);
  border-radius: 40px;
}

@C-Lodder Thanks.

@ravisaxena23 Could you impement C-Lodder's suggestions?

• Use a variable for the z-index, see #27705 (comment)
• Change order of SCSS properties, see #27705 (comment)

Thanks in advance.

Sure

@richard67 @C-Lodder done that

@richard67 please review!

Suggested changes have been implemented.

Now it needs 2 good tests, then all is fine. Will test later today or tomorrow. Am busy with something else now.

I have tested this item ✅ successfully on 811a473

Testing instructions:

1. Go to global configuration in backend and reduce screen width to the minimum possible.
2. Toggle the bottom round blue button with cross icon (should be a burger icon instead).
   Result: Icon is always cross, does not change depending on expanded status of the admin menu at the bottom.
3. Provoke an alert to be shown, e.g. by entering invalid database name and trying to save.
   Result: Alert is shown, but button to close it is hidden by the "gears" button.
4. Apply the patch of this PR and then run "npm i".
5. Clear broswer cache.
6. Repeat steps 1 to 3.
   Results: The icon of the "burger" button changes as expected when toggling the button. It shows a cross icon to collapse the menu when the menu is expanded, and a burger icon when the menu is collapsed. The alert comes in front of the "gears" button so the cross button to close the alert is not hidden anymore.

I have tested this item ✅ successfully on a7c26c2

Last 2 commits were only code style, so my previous test is still valid.

Hint for other testers: More detailed testing instructions than in the description see my previous test result above.

I have tested this item ✅ successfully on 7458195

Previous test is still valid, last merge was just a clean update to the base branch.

Hint for other testers: More detailed testing instructions than in the description see my previous test result above.

I have tested this item ✅ successfully on 7458195

RTC

I have not tested this item.

Sorry, haven't tested with RTL languages. Will do so soon.

@infograf768 You are right, it breaks RTL.

Before PR it is ok:

After PR the gears button is broken, i.e. still at the right hand side:

I have tested this item 🔴 unsuccessfully on 7458195

For LTR it is ok, but RTL is broken for the gears button, see screenshots in my previous comment.

@ravisaxena23 Do you think you can fix that?

I have tested this item 🔴 unsuccessfully on 7458195

For LTR it is ok, but RTL is broken for the gears button, see screenshots in my previous comment.

@ravisaxena23 Do you think you can fix that?

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

@richard67 I hope I have corrected RTL issue please test again.

Code would be

@include media-breakpoint-down(xs) {
  .toggler-toolbar {
    position: fixed;
    top: 20px;
    bottom: 20px;
    z-index: $zindex-alerts;
    display: block;
    width: 70px;
    height: 70px;
    background: var(--atum-link-color);
    border: 8px solid var(--atum-bg-light);
    border-radius: 40px;

    [dir=ltr] & {
      right: 10px;
    }
 
    [dir=rtl] & {
      left: 10px;
    }

to get

[dir=ltr] & {
right: 10px;
}

[dir=rtl] & {
  left: 10px;
}

it was working fine whit LTR so i jus added code for RTL should i add code for ltr too? @infograf768

@ravisaxena23

It is better in that case and more legible to add both the ltr and rtl separated as it concerns only one aspect, so one line change instead of using auto.

Evidently, right:10px; being added in

[dir=ltr] & {
right: 10px;
}

is to be taken off in the main class above

@ravisaxena23

It is better in that case and more legible to add both the ltr and rtl separated as it concerns only one aspect, so one line change instead of using auto.

@infograf768 Done it Please check.

@ravisaxena23

It is better in that case and more legible to add both the ltr and rtl separated as it concerns only one aspect, so one line change instead of using auto.

@infograf768 done that please review again

why spaces become issue? what problem they cause? @infograf768 please explain and Check Pr again

why spaces become issue? what problem they cause?

No issue, just code style. A blank line is blank, no tabs no spaces.

why spaces become issue? what problem they cause?

No issue, just code style. A blank line is blank, no tabs no spaces.

@infograf768 please review again. and please help me out I'm unable to see space in my VS Code. also i was thinking to add prettier as a script it can format code then we dont have to bother for spaces but it may change all code format present right now. how is the idea? can we work on it?

@infograf768 done that too. sorry forget we don't need it anymore after rtl and ltr

Looks good now, will test again after dinner. Here is 8 PM now ;-)

I have tested this item ✅ successfully on a85c4c1

RTL working now, too.

@drmenzelit Could you test again, for LTR and also for RTL? For RTL you can use Persian language.

I have tested this item ✅ successfully on a85c4c1

rtc

@ravisaxena23 Congratulations, your PR is ready to commit (RTC) now. Now you better just wait and don't touch it. A maintainer will merge it when appropriate. Only do something when we ping you here for solving conflicts, which may happen when someone else meanwhile changes a file which you have changed, too, and that other change is committed before yours, then this can happen. But as long as it's not the case, leave things as they are now. All fine and well done finally.

Merged. thanks.

Sorry but this is not correct - see #27805