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

[Admin] Added collapse menu #11668

Merged
merged 1 commit into from Jul 22, 2020
Merged

[Admin] Added collapse menu #11668

merged 1 commit into from Jul 22, 2020

Conversation

TefDesign
Copy link

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets -
License MIT

Add collapse on admin sidebar

sylius_admin_menu

@TefDesign TefDesign requested a review from a team as a code owner July 21, 2020 11:53
@probot-autolabeler probot-autolabeler bot added the Admin AdminBundle related issues and PRs. label Jul 21, 2020
lchrusciel
lchrusciel approved these changes Jul 22, 2020
View reviewed changes
Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

Hey Stéphane,

thanks a lot for this improvement :) Also, welcome among Sylius contributors 🎉

@lchrusciel lchrusciel added the UX Issues and PRs aimed at improving User eXperience. label Jul 22, 2020
@lchrusciel
Copy link
Member

However, can you rebase to the newest master and check how will it behave together with #11571?

@lchrusciel
Copy link
Member

Also, will it remember which cards have been collapsed and which not? It could be annoying for users to always toggle them.

@TefDesign
Copy link
Author

TefDesign commented Jul 22, 2020 •
edited

Hi @lchrusciel
Thanks :)
For #11571 , it's already present in my pull request 😃 :
image

The first screenshot was not up to date sorry :P

I created my pull request from the master so the task is there :)
For your question, I am using the "current_ancestor" class already present and I have not touched the html structure. With this class, if an item is active, the entire block remains open.
In the JavaScript function, I have provided a parameter that allows opening a menu to close the others if a user prefers this system. All JavaScript script is documented in JSDoc standards !

@TefDesign
Copy link
Author

@lchrusciel I will still check something and I tell you :)

@TefDesign
Copy link
Author

@lchrusciel Rebase ok and check ok 👍 :
image

@lchrusciel lchrusciel merged commit e5ec97a into Sylius:master Jul 22, 2020
@lchrusciel
Copy link
Member

Thanks, Stéphane! 🎉

@CoderMaggie CoderMaggie mentioned this pull request Jul 23, 2020
Merged
pamil added a commit that referenced this pull request Jul 23, 2020
@pamil
bug #11674 Revert "[Admin] Added collapse menu" (CoderMaggie)
3f23da0 
This PR was merged into the 1.8-dev branch.

Discussion
----------

Reverts #11668

Although it's nice to see new contributions and I am very grateful for the community input, this PR has to be reverted. 😢 


![Nagranie z ekranu 2020-07-23 o 08 34 23](https://user-images.githubusercontent.com/13198869/88258296-1c5f0d80-ccc0-11ea-9d91-f3deeeafe46d.gif)


The behaviour of this "menu collapsing JS" highly lowers the UX of the admin menu. 
First of all, it works slowly. Secondly, it is collapsed by default, so after each redirect, we end up with a menu collapsed again even though we've been using it a moment ago. And finally, it was merged pretty reckless, without consulting in the community and with the Core Team.

Final thought, for anyone who'd like to rework that: The admin menu should be collapsible, not collapsed. 😂 

Commits
-------

9baca24 Revert "[Admin] Added collapse menu"
vasilvestre
vasilvestre reviewed Nov 10, 2020
View reviewed changes
src/Sylius/Bundle/AdminBundle/Resources/private/sass/_ui.scss
visibility: hidden;
overflow: hidden;
max-height: 0;
transition: all .5s ease-in-out;
Copy link
Contributor

Choose a reason for hiding this comment

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

https://medium.com/outsystems-experts/how-to-achieve-60-fps-animations-with-css3-db7b98610108
https://stackoverflow.com/a/12347702/6796253
These links seem to fix the actual problem. :)

@vasilvestre vasilvestre mentioned this pull request Dec 29, 2020
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vasilvestre vasilvestre vasilvestre left review comments

@chadyred chadyred chadyred approved these changes

@lchrusciel lchrusciel lchrusciel approved these changes

Assignees
No one assigned
Labels
Admin AdminBundle related issues and PRs. UX Issues and PRs aimed at improving User eXperience.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@TefDesign @lchrusciel @chadyred @vasilvestre @StefDedi