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

Fix admin left menu dynamic visibility #1157

Merged
merged 2 commits into from
May 2, 2020
Merged

Fix admin left menu dynamic visibility #1157

merged 2 commits into from
May 2, 2020

Conversation

mvorisek
Copy link
Member

@mvorisek mvorisek commented May 1, 2020

Fix issue intoduced in #1112. Visibility is controlled by isMenuLeftVisible prop and needs to be not always forced to visible.

@codecov
Copy link

codecov bot commented May 1, 2020

Codecov Report

Merging #1157 into develop will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #1157   +/-   ##
==========================================
  Coverage      75.77%   75.77%           
  Complexity      2539     2539           
==========================================
  Files            130      130           
  Lines           6230     6230           
==========================================
  Hits            4721     4721           
  Misses          1509     1509           
Impacted Files Coverage Δ Complexity Δ
src/Layout/Admin.php 89.65% <100.00%> (ø) 11.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24df58c...a3dae7e. Read the comment docs.

@ibelar
Copy link
Contributor

ibelar commented May 1, 2020

let's merge #1151 first and see what can be done after.
Bringing the z-index lower for the left menu should fix the border state in #1156

@mvorisek
Copy link
Member Author

mvorisek commented May 1, 2020

This visibility fix is unrelated with any current PRs!

@mvorisek mvorisek requested a review from ibelar May 1, 2020 18:14
@mvorisek
Copy link
Member Author

mvorisek commented May 1, 2020

@ibelar PR is complete and fully working, please check and merge

@mvorisek mvorisek requested a review from DarkSide666 May 2, 2020 14:23
Copy link
Contributor

@ibelar ibelar left a comment

Choose a reason for hiding this comment

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

Look good now!

@ibelar ibelar merged commit ceddc73 into atk4:develop May 2, 2020
@mvorisek mvorisek deleted the fix_left_menu_dynamic_visibility branch May 2, 2020 15:11
@mvorisek
Copy link
Member Author

@ibelar it does fixed the original switching behaviour, but if default to on, the menu "flashes" between loads which degrades the UX. Can you fix this in server-side render?

@georgehristov
Copy link
Collaborator

georgehristov commented May 15, 2020

I have below code in my custom layout to control the state of navigation menu. It basically persists the state in the session. So basically adding the SessionTrait to Layout and using following code.
Good solution? Shall I make a PR?

$this->isMenuLeftVisible = $this->learn('menu', $this->isMenuLeftVisible);
        
$this->burger->add(['Icon',	'content'])->on('click', [
        		(new jQuery('.ui.left.sidebar'))->toggleClass('visible'),
        		(new jQuery('.epesi-logo'))->toggleClass('expanded'),
        		(new jQuery('body'))->toggleClass('atk-leftMenu-visible'),
        		$this->burger->add('jsCallback')->set(function($j, $visible) {
        			$this->memorize('menu', filter_var($visible, FILTER_VALIDATE_BOOLEAN));
        		}, [$this->menuLeft->js(true)->hasClass('visible')])
)])
]);

@mvorisek
Copy link
Member Author

mvorisek commented May 15, 2020

@georgehristov it is not about persisting the state - it is about adding/removing visible class on the server side thus the browser can render the menu immediatelly which will prevent the flashing introduced by solving this in the JS - which takes some time before the JS is triggered/applied. Your code does not solve this (not-using the JS).

@georgehristov
Copy link
Collaborator

I see now. I do not have this problem as I use routing where only the content area of the page is updated on page change.

@mvorisek
Copy link
Member Author

@georgehristov do you solve there a history? #1191

@georgehristov
Copy link
Collaborator

Yes, there is a browser history. I have not had problem with that.

@mvorisek
Copy link
Member Author

I see now. I do not have this problem as I use routing where only the content area of the page is updated on page change.

I am lost, you either reload the full page - and this issue is affecting you too - or you use JS to reload the inner component, but then need the history API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants