Skip to content

Fixing infinite spinning animation on user menu ctrl+click #778

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
Merged
MorrisJobke merged 5 commits into from
Aug 18, 2016
Merged

Fixing infinite spinning animation on user menu ctrl+click #778

MorrisJobke merged 5 commits into from
Aug 18, 2016

Conversation

@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Aug 9, 2016

Thomas Pulzer added 3 commits August 9, 2016 11:58
@MorrisJobke
Fixed user menu to not show spinner on ctrl+click.
1ab9477 
Fixes #616
@MorrisJobke
Added OS X meta key check for stopping spinning animation in app and …
8c9961a 
…user menu.
@MorrisJobke
Changed app and user menu delegates to mousedown events.
c503367 
Added proper handling of primary mouse button click with and without ctrl-/meta-key modifier.
Added handlig of middle mouse button click.
@MorrisJobke
Copy link
Member Author

MorrisJobke commented Aug 9, 2016

@Faldon Does this look right? Could you check this out and retry?

What I did to get this:

  • fetch latest master
  • checkout your branch
  • do a git rebase master
  • there where two conflicts in core/js/js.js -> resolve them (both resulted in empty changeset and I needed to do a git rebase --skip to skip the resulting empty commits - this was told me by the git command line interface):
$ git rebase --continue 
Applying: Added OS X meta key check for stopping spinning animation in app and user menu.
No changes - did you forget to use 'git add'?
If there is nothing left to stage, chances are that something else
already introduced the same changes; you might want to skip this patch.

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

@MorrisJobke MorrisJobke mentioned this pull request Aug 9, 2016
Closed
@MorrisJobke MorrisJobke added this to the Nextcloud 11.0 milestone Aug 9, 2016
@MorrisJobke
Copy link
Member Author

MorrisJobke commented Aug 9, 2016

cc @icewind1991 @schiessle @jancborchardt @ChristophWurst @juliushaertl

@Faldon
Copy link
Contributor

Faldon commented Aug 9, 2016

Checked out and it works as expected. Thanks for helping out. Have to practice the rebase a little bit more.

@schiessle
Copy link
Member

schiessle commented Aug 9, 2016

looks good 👍

@MorrisJobke
Copy link
Member Author

MorrisJobke commented Aug 9, 2016

The menu now closes on right click. So the context menu of the right click hang somewhere in the UI, but the right clicked element is not visible anymore. @Faldon Is this the wanted behaviour?

@Faldon
Copy link
Contributor

Faldon commented Aug 10, 2016

I personally prefer it that way. Although it makes debugging a little bit harder.
But I understand that not everyone expects the menu closes on right click. So, it is the wanted behavior by the programmer, not necessarily the wanted behavior by the community. :-)

@jancborchardt
Copy link
Member

jancborchardt commented Aug 10, 2016

It shouldn’t close on right-click, doesn’t do that at the moment either. ;) Let’s stick to fixing the one issue at hand and not putting other stuff in.

Fixed user and main menu closing on right mouse button click.
b2b7fa4 
Fixed wrong variable assignment when trying to open link in new window.
@Faldon
Copy link
Contributor

Faldon commented Aug 11, 2016

Ok. I rewrote it to stay open and not showing the spinner animation when doing a right click on the menu.

MorrisJobke
MorrisJobke reviewed Aug 11, 2016
View reviewed changes
core/js/js.js Outdated
// a new tab
OC.hideMenus();
// On middle click or on first button click with ctrl key or meta key hold
console.log(event.which);
Copy link
Member Author

@MorrisJobke MorrisJobke Aug 11, 2016

Choose a reason for hiding this comment

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

debug log? ;)

@Faldon
Copy link
Contributor

Faldon commented Aug 11, 2016

Ach f* -.-
On the run atm, but will fix it this evening.

@MorrisJobke
Copy link
Member Author

MorrisJobke commented Aug 11, 2016

Ach f* -.-On the run atm, but will fix it this evening.

No worries 😄

@juliusknorr
Copy link
Member

juliusknorr commented Aug 16, 2016

works fine now 👍

@juliusknorr
Copy link
Member

juliusknorr commented Aug 17, 2016

@MorrisJobke can we merge this or is it required to have another reviewer, as there were some more changes after @schiessle gave his thumbs up?

@MorrisJobke
Copy link
Member Author

MorrisJobke commented Aug 18, 2016

@MorrisJobke can we merge this or is it required to have another reviewer, as there were some more changes after @schiessle gave his thumbs up?

I will test this again and review it finally.

@MorrisJobke
Copy link
Member Author

MorrisJobke commented Aug 18, 2016

Tested and works now as wanted 👍

@MorrisJobke MorrisJobke merged commit 3e7710c into master Aug 18, 2016
@MorrisJobke MorrisJobke deleted the Faldon-user_menu_ui branch August 18, 2016 06:40
@MorrisJobke
Copy link
Member Author

MorrisJobke commented Aug 18, 2016

I would keep this as it is in master and not backport this. @jancborchardt is this okay for you?

@MorrisJobke
Copy link
Member Author

MorrisJobke commented Aug 18, 2016

This kills for me and @nickvergessen navigation between apps (for Joas all apps, for me only gallery -.-) in Firefox. I will revert this now.

@MorrisJobke MorrisJobke mentioned this pull request Aug 18, 2016
Merged
@MorrisJobke
Copy link
Member Author

MorrisJobke commented Aug 18, 2016

Reverted with #929

@Faldon Could you create your PR again and only solve it step by step. I guess the issue here was the "open with middleclick" thing. For now it would be enough to first mimic the current apps menu behavior and fix the middle click in a separate PR.

@Faldon
Copy link
Contributor

Faldon commented Aug 18, 2016

I will look into it.

@MorrisJobke
Copy link
Member Author

MorrisJobke commented Aug 18, 2016

I will look into it.

Thanks a lot 👍

Faldon pushed a commit to Faldon/server that referenced this pull request Aug 19, 2016
Fixing infinite spinner animation
35bddb3 
setupMainMenu() & setupUserMenu():
Changed click delegate to add the spinner animation only the primary mouse button was clicked without ctrl- or meta-key modifier
Adding mouseup delegate to hide the menu if the middle mouse button was clicked.

Redone nextcloud#778
Faldon pushed a commit to Faldon/server that referenced this pull request Aug 19, 2016
Fixing infinite spinner animation
58f9340 
setupMainMenu() & setupUserMenu():
Changed click delegate to add the spinner animation only the primary mouse button was clicked without ctrl- or meta-key modifier
Adding mouseup delegate to hide the menu if the middle mouse button was clicked.

Redone nextcloud#778
@Faldon Faldon mentioned this pull request Aug 19, 2016
Merged
@Faldon
Copy link
Contributor

Faldon commented Aug 19, 2016

@MorrisJobke So, I created a new PR to have a clean base.

GitHubUser4234 pushed a commit to GitHubUser4234/server that referenced this pull request Aug 30, 2016
@MorrisJobke
Merge pull request nextcloud#778 from nextcloud/Faldon-user_menu_ui
29b5888 
Fixing infinite spinning animation on user menu ctrl+click
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

3. to review Waiting for reviews

Projects

None yet

Milestone

Nextcloud 11.0

Development

Successfully merging this pull request may close these issues.

6 participants

@MorrisJobke @Faldon @schiessle @jancborchardt @juliusknorr