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.

Sign up for GitHub

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

Jump to bottom

Fix the mobile menu wrongly expanding the logo #33877

Merged
merged 14 commits into from
Jun 10, 2021
Merged

Fix the mobile menu wrongly expanding the logo #33877

merged 14 commits into from
Jun 10, 2021

Conversation

dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented May 14, 2021 •
edited
Loading

Pull Request for Issue #33754 (comment) and #34105

Summary of Changes

Joomla logo is expanded pushing the cog icon out of view and toolbar options are displayed by default.

Testing Instructions

Shrink the width of your browser to a mobile width and open the menu/toolbar. The logo should not expand

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels May 14, 2021
@dgrammatiko dgrammatiko mentioned this pull request May 14, 2021
Merged
@Quy
Copy link
Contributor

Quy commented May 14, 2021

It is fine/collapsed on the Home Dashboard initially, but going to other sections such as Articles list, it expands again.

@RickR2H
Copy link
Member

RickR2H commented May 16, 2021

When I do a refresh in Chrome with the mobile view active, the logo is initially expanded. When I resize the mobile window just a little bit, the logo shrinks and everything jumps into place. When I do a refresh again the logo is back to its initial expanded state.

@dgrammatiko dgrammatiko mentioned this pull request May 22, 2021
Closed
@dgrammatiko
Merge branch '4.0-dev' of github.com:joomla/joomla-cms into —handle-s…
28de4bb 
…idebar-better—4.0-dev

# Conflicts:
#	build/media_source/templates/administrator/atum/js/template.es6.js
@dgrammatiko
a
a5b1776
@dgrammatiko
Fix it
b0d82e0
@PhilETaylor

This comment was marked as abuse.

Sign in to view
@PhilETaylor

This comment was marked as abuse.

Sign in to view
@PhilETaylor

This comment was marked as abuse.

Sign in to view
@PhilETaylor

This comment was marked as abuse.

Sign in to view
@PhilETaylor

This comment was marked as abuse.

Sign in to view
@PhilETaylor

This comment was marked as abuse.

Sign in to view
@dgrammatiko
Copy link
Contributor Author

What I will say, is that the web is meant to be browsed, not squished.

There are still assumptions with the current code, especially for the transition to mobile or going from mobile to desktop.

Could we tackle those?

Probably yes by transforming the cookie value to a JSON string (store state for logo, menu state and header toolbar), but I see little advantage to complicate further the code to iron out a couple of glitches which are edge cases (e.g. clicking on the save button on mobile view will not reload the page with the toolbar open). There are trade-offs (perf vs covering all cases), and as is the script is not very efficient/performant, and my whole approach here was to eliminate as much as possible the annoying open/close animations on page reload and not to re-architecture the way we put together those underlaying parts

@PhilETaylor

This comment was marked as abuse.

Sign in to view
@dgrammatiko dgrammatiko mentioned this pull request May 24, 2021
Closed
eopws
eopws reviewed May 24, 2021
View reviewed changes
build/media_source/templates/administrator/atum/js/template.es6.js
@@ -217,11 +222,23 @@ function subheadScrolling() {
headerItemsInDropdown();
reactToResize();
subheadScrolling();
if (mobile.matches) {
Copy link

@eopws eopws May 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Why not using setMobile function call inside this block?

@YatharthVyas YatharthVyas mentioned this pull request May 24, 2021
Closed
@Quy
Copy link
Contributor

Quy commented May 28, 2021

I have tested this item ✅ successfully on cdfbb5f

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

@dgrammatiko dgrammatiko mentioned this pull request Jun 1, 2021
Closed
@PhilETaylor

This comment was marked as abuse.

Sign in to view
@Quy
Copy link
Contributor

Quy commented Jun 1, 2021

RTC

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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 1, 2021
@Quy Quy added this to the Joomla 4.0 milestone Jun 1, 2021
@chmst chmst merged commit 6ff0fc7 into joomla:4.0-dev Jun 10, 2021
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 10, 2021
@chmst
Copy link
Contributor

chmst commented Jun 10, 2021

Thanks!

@dgrammatiko dgrammatiko deleted the patch-1 branch June 10, 2021 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eopws eopws eopws left review comments

Assignees
No one assigned
Labels
NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Milestone
Joomla 4.0
Development

Successfully merging this pull request may close these issues.
7 participants
@dgrammatiko @Quy @RickR2H @PhilETaylor @chmst @eopws @joomla-cms-bot