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

[4.0]fix error of twice clicking on toolbar and its disappearance on resizing #27958

Closed
wants to merge 9 commits into from
Closed

Conversation

Subhang23
Copy link
Contributor

@Subhang23 Subhang23 commented Feb 17, 2020

Pull Request for Issue #26543 and #27957 .

Summary of Changes

Added class to the div so that on loading it stays in either collapsed form or not-collapsed form(previously it was in a state of mix).
Added querySelectors to ensure that whenever screen size is that of desktop the classes of toolbar stays the same.

Testing Instructions

Go to any page which has a toolbar and reduce the width of the screen to a mobile size.

Expected result

You can open or close the toolbar with a single click and on resizing and closing the toolbar and again resizing the toolbar is intact.

Actual result

With the initial state as pulled up only one click is required. Even on removing toolbar in mobile-view and resizing the toolbar is seen

Documentation Changes Required

@Subhang23
Copy link
Contributor Author

Screenshot from 2020-02-17 11-39-17

@SharkyKZ
Copy link
Contributor

Now the toolbar is missing on large screens.

@Subhang23
Copy link
Contributor Author

I understood the error it was because if the toolbar is compacted in mobile-view it is hidden in desktop-view. This might also be the reason for Issue #27957 .

@Subhang23 Subhang23 changed the title fix error of twice clicking on toolbar [4.0]fix error of twice clicking on toolbar Feb 17, 2020
@infograf768
Copy link
Member

Looks like working.
Now the toolbar is showing on desktop
Note: If the screen is reduced, the toolbar closed and then the screen enlarged to desktop, it requires a reload to get the toolbar again.
Issue or not?

@SharkyKZ
Copy link
Contributor

That's the issue reported in #27957.

@Subhang23
Copy link
Contributor Author

That's the issue reported in #27957.

I am currently working on it.

@Subhang23
Copy link
Contributor Author

Looks like working.
Now the toolbar is showing on desktop
Note: If the screen is reduced, the toolbar closed and then the screen enlarged to desktop, it requires a reload to get the toolbar again.
Issue or not?

Is this fine?

@infograf768
Copy link
Member

Is this fine?

When #27957 is solved, yes.

@Subhang23
Copy link
Contributor Author

So should I close this PR and open a PR that solves this issue and Issue #27957 together. I thought they can be solved separately, so I thought of opening 2 separate PR's.
What should I do @infograf768 ?

@infograf768
Copy link
Member

I am not the one to decide.
If you can solve it together here or in a new PR, it is easier on testers.

@joomla-cms-bot joomla-cms-bot added the NPM Resource Changed This Pull Request can't be tested by Patchtester label Feb 17, 2020
@Subhang23
Copy link
Contributor Author

@infograf768 I have fixed both the issues with this PR. Please review it.

@brianteeman
Copy link
Contributor

Looks good to me

@Subhang23 Subhang23 changed the title [4.0]fix error of twice clicking on toolbar [4.0]fix error of twice clicking on toolbar and its disappearance on resizing Feb 17, 2020
@infograf768
Copy link
Member

I have tested this item ✅ successfully on a02351f

Great!


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

@Subhang23
Copy link
Contributor Author

@C-Lodder I have made the required changes.

@C-Lodder
Copy link
Member

So right now, every time you change the viewport size by 1px, you're going to be searching to DOM 5 times. Not very good for performance.

@C-Lodder
Copy link
Member

@Subhang23 targetting DOM elements should go outside of the resize event listener.

@Subhang23
Copy link
Contributor Author

@wilsonge please review it

window.addEventListener('resize', () => {
if (window.innerWidth >= 576) {
if (toolbar && !toolbar.querySelector('#subhead').classList.contains('show')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In cpanel #subhead doesn't exist and this causes a JS error getting the property classList of null

@wilsonge
Copy link
Contributor

wilsonge commented Apr 4, 2020

This doesn't work for me. First of all it totally fails on cpanel (see comment above).

When the menu is open and i leave mobile menu it stays open. However when the mobile menu is hidden it doesn't reappear at all. See screenshot - there's no way to reopen the menu at all
Screenshot 2020-04-04 at 17 49 57

@alikon
Copy link
Contributor

alikon commented Apr 4, 2020

same from my test

@Subhang23
Copy link
Contributor Author

I am not sure if this is the right place to ask but I have run into a problem while setting up Joomla on my local server
Screenshot from 2020-04-05 01-50-32
I am running it on Ubuntu 19.04 php7.3

@wilsonge
Copy link
Contributor

wilsonge commented Apr 8, 2020

not sure what's going on there :( looks very sad. are you sure Joomla is even loading and it's not something from the web server. all our messages should have more info than that

@Quy Quy added Updates Requested Indicates that this pull request needs an update from the author and should not be tested. Ready to take over This is an abandoned feature which can be taken over by another person to finish it. labels May 25, 2020
@particthistle
Copy link
Member

I have tested this item 🔴 unsuccessfully on 32c00cb

Patch was unable to be applied. Receiving:

Error
The patch could not be applied because the repository is missing

@Subhang23 if you're still having issues with your local installation, read https://magazine.joomla.org/all-issues/june-2020/github-installing-git which might reveal something you've done wrong with your local installation, though this article outlines XAMPP not Ubuntu specifically.


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

@Quy
Copy link
Contributor

Quy commented Jul 17, 2020

Please test PR #30131

@Quy Quy closed this Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NPM Resource Changed This Pull Request can't be tested by Patchtester Ready to take over This is an abandoned feature which can be taken over by another person to finish it. Updates Requested Indicates that this pull request needs an update from the author and should not be tested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants