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 delay initialization options for customized JQuery UI menu widget #12161

Conversation

scazz010
Copy link

Description

Documentation: http://devdocs.magento.com/guides/v2.2/javascript-dev-guide/widgets/widget_menu.html

describes the ability to set the delay on the JQuery widget opening/closing with an option 'delay'.

However, in the code, no such option appears and two undocumented options that aren't used do appear.

This PR cleans up the unused variables and passes the delay parameter into JQuery menu widget so it is used.

Manual testing scenarios

  • Navigate to the home page
  • Hover over main navigation tab. Observe 300ms for widget menu to open/expand
  • Edit vendor/magento/module-theme/view/frontend/templates/html/topmenu.phtml - change UL tag to be:

<ul data-mage-init='{"menu":{"delay": 800, "responsive":true, "expanded":true, "position":{"my":"left top","at":"left bottom"}}}'>

  • Hover over main navigation tab. Observe 800ms delay for menu to open/expand

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Nov 10, 2017

CLA assistant check
All committers have signed the CLA.

@ihor-sviziev
Copy link
Contributor

@scazz010 static test failed. Could you fix your code?

@ihor-sviziev ihor-sviziev self-assigned this Nov 10, 2017
@scazz010 scazz010 force-pushed the delay-on-jquery-ui-menu-not-overwritten branch from c7eacf1 to ab06abe Compare November 10, 2017 18:18
@scazz010
Copy link
Author

Eeek, sorry! grunt static is clean again

@@ -19,8 +19,7 @@ define([
options: {
responsive: false,
expanded: false,
showDelay: 42,
hideDelay: 300,
delay: 300,
Copy link
Contributor

Choose a reason for hiding this comment

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

Original options that were removed still used on 104 and 108 lines in this file, so after removal them from options - they will be not defined. It looks like you've added regression. Could you check it?

@scazz010 scazz010 force-pushed the delay-on-jquery-ui-menu-not-overwritten branch from ab06abe to 6330103 Compare November 14, 2017 18:00
@scazz010
Copy link
Author

scazz010 commented Nov 14, 2017

Hi Ihor,

You're 100% correct.

The hideDelay option is tied to the CSS animations to animate the menu in responsive mode. The hideDelay needs to be at least as long as the animation to hide the menu otherwise the menu toggle button appears during the transition. I guess this should be public as people may want to customize the css delay.

showDelay is just a pause between clicking the toggle menu button and the animation starting - I can't see any issues with it being 0 and I can't see a reason for a user wanting to pause for a long time before beginning that animation. I assume there's a device that has issues without the delay and that's why it's there. 42ms seems weirdly arbitrary to me - I just can't see a reason for anyone to want to change that value, but I guess we can't break the API by changing the functionality and hardcoding/setting it to private?


I've put the showDelay and hideDelay options back in and left my delay option too. I also removed a hardcoded value. I don't particularly like the naming - I would love to change showDelay and hideDelay to something like responsiveShowDelay and responsiveAnimationHideDelay but that would break the API and I guess the PR won't be accepted. I've kept mine as delay because that's what it's called inside jQuery and I think it makes the most sense.

Would love feedback,
Thanks
S

@omiroshnichenko omiroshnichenko added this to the November 2017 milestone Nov 24, 2017
@omiroshnichenko
Copy link
Contributor

@scazz010 Could you please sign CLA?

@scazz010
Copy link
Author

@omiroshnichenko Signed!

@omiroshnichenko
Copy link
Contributor

@scazz010 Looks like you made cherry-pick of commit that was made from this stemcellscarr account. Can you sign CLI from another account too?

@scazz010
Copy link
Author

@omiroshnichenko done!

@magento-team magento-team merged commit faa7129 into magento:2.2-develop Dec 1, 2017
magento-team pushed a commit that referenced this pull request Dec 1, 2017
@scazz010 scazz010 deleted the delay-on-jquery-ui-menu-not-overwritten branch December 1, 2017 19:39
@ihor-sviziev ihor-sviziev removed their request for review March 12, 2018 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants