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

Fixes #25833: increase hover delay for menu items. #6405

Merged
merged 1 commit into from
Mar 5, 2019

Conversation

waldenraines
Copy link

Ensure menu items don't close too quickly by increasing the delay.

https://projects.theforeman.org/issues/25833

@theforeman-bot
Copy link
Member

Issues: #25833

@waldenraines
Copy link
Author

[test foreman]

Copy link
Member

@amirfefer amirfefer left a comment

Choose a reason for hiding this comment

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

Thanks @waldenraines 👍

The delay feels longer than expected to me
ezgif com-video-to-gif 1

how about decreasing it a bit?

@waldenraines
Copy link
Author

Anyone have any thoughts on this?

Stacktrace
Capybara::Poltergeist::JavascriptError: One or more errors were raised in the Javascript code on the page. If you don't care about these errors, you can ignore them by setting js_errors: false in your Poltergeist configuration (see documentation for details).

[sprintf] huh?
[sprintf] huh?
    at http://127.0.0.1:42252/webpack/vendor-7072f429e647f40ef323.js:246 in vo
    test/integration/host_js_test.rb:539:in `block (2 levels) in <class:HostJSTest>' (Capybara::Poltergeist::JavascriptError)
/usr/local/rvm/gems/ruby-2.4.3@test_develop_pr_core-0/gems/poltergeist-1.18.1/lib/capybara/poltergeist/browser.rb:396    

Umm, what?

@waldenraines
Copy link
Author

The delay feels longer than expected to me
how about decreasing it a bit?

I suppose we could, I just wasn't sure what the best value was so as to not get the bug reopened. @Rohoover do you have any thoughts?

Copy link
Member

@chris1984 chris1984 left a comment

Choose a reason for hiding this comment

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

What about 50 instead of 100?

@Rohoover
Copy link

Hey, I tried this out on the dogfood server and I personally didn't experience any miss-clicks from delay. It does seem a bit longer than expected in the video above.

@waldenraines
Copy link
Author

Hey, I tried this out on the dogfood server and I personally didn't experience any miss-clicks from delay. It does seem a bit longer than expected in the video above.

I think the dogfood server would have the standard version of what is in master (oops, develop 🤦‍♂️). You can try this out on my sandbox if you want, just message me and I'll give you the details.

@ohadlevy
Copy link
Member

ohadlevy commented Jan 16, 2019 via email

@ares
Copy link
Member

ares commented Feb 19, 2019

[test foreman] perhaps you'll need to rebase

@waldenraines
Copy link
Author

[test katello]

@waldenraines
Copy link
Author

Is this a known issue in travis?

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory

Ensure menu items don't close too quickly by increasing the delay.

https://projects.theforeman.org/issues/25833
@ohadlevy
Copy link
Member

ohadlevy commented Feb 20, 2019 via email

@coveralls
Copy link

Coverage Status

Coverage remained the same at 70.062% when pulling 071ab90 on waldenraines:25833 into 870d459 on theforeman:develop.

@waldenraines
Copy link
Author

[test foreman]

@ohadlevy
Copy link
Member

ohadlevy commented Mar 1, 2019

whats the status of this pr?

@Rohoover
Copy link

Rohoover commented Mar 4, 2019

@waldenraines

Got a chance to test the new delay. I like it quite a bit! Nice job!

@waldenraines
Copy link
Author

whats the status of this pr?

Should just need to be reviewed/merged.

Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

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

Much better experience. For things like that I'd like to have more opinions, but given we have ack from @Rohoover and how complicated the patch or the delay adjustment is, I'll go ahead and merge.

Thanks @waldenraines, merging!

@ares ares merged commit dce0dc2 into theforeman:develop Mar 5, 2019
@waldenraines waldenraines deleted the 25833 branch March 5, 2019 15:53
@ezr-ondrej
Copy link
Member

It is introducing this regression:
Regresion
My wild guess is that if the Audits start to render too soon, they got re-rendered after the timeout 🤷‍♂️ but not really sure.

@waldenraines
Copy link
Author

It is introducing this regression:

I can barely tell what is happening in the gif, can you explain it please?

@ezr-ondrej
Copy link
Member

Yeah sorry, the Audits page in not loading - is blank, the component is rendered, but not visible. I do not know why, but i went commit by commit and this is the first where it happens for me.

@tbrisker
Copy link
Member

@ezr-ondrej - I'm seeing the audits page fail to load when I have an audit that is missing UserInfo, with the following error in the console:

Warning: Failed prop type: The prop `userInfo.search_path` is marked as required in `UserDetails`, but its value is `undefined`.

When I comment out the required props in the UserDetails component, the page loads just fine.

@ares
Copy link
Member

ares commented Mar 12, 2019

So, what's the status here? Should we revert until we have the fix ready? Did someone open a tracking issue already? I suppose that should be 1.22 blocker.

@tbrisker
Copy link
Member

https://projects.theforeman.org/issues/26286 is for the issue I've uncovered, I'm not sure this change is related to it. Perhaps the latest audits changed while trying to bysect leading to this commit?

@waldenraines
Copy link
Author

https://projects.theforeman.org/issues/26286 is for the issue I've uncovered, I'm not sure this change is related to it. Perhaps the latest audits changed while trying to bysect leading to this commit?

Yeah, I don't see how it could be related. The delay I added doesn't delay the loading of the javascript or anything like that and with the JS error @tbrisker mentions above I think it really seems unrelated.

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