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

Add context menus #9

Merged
merged 1 commit into from
Mar 6, 2016
Merged

Add context menus #9

merged 1 commit into from
Mar 6, 2016

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Feb 12, 2016

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Feb 14, 2016
@MaxLeiter
Copy link
Member

OS: Fedora 23
Browser: Chrome Version 48.0.2564.109 (64-bit)

The context menu opens fine but the buttons have no action and the menu closes as soon as I try clicking an option. Investigating now.

@xPaw
Copy link
Member Author

xPaw commented Feb 17, 2016

@MaxLeiter Try setting higher z-index on #context-menu, it probably detects mousedown on container before it gets to click event on the menu itself.

@MaxLeiter
Copy link
Member

No effect @xPaw, tried multiple different values (including the maximum for z-index), no effect

@MaxLeiter
Copy link
Member

Worked great with the fix, 👍

@astorije
Copy link
Member

(Can be rebased off master because client/js/lounge.templates.js is not necessary anymore)

@AlMcKinlay AlMcKinlay self-assigned this Feb 19, 2016
@AlMcKinlay
Copy link
Member

Righty, so I'm really liking this.

However, just a little thing, we probably shouldn't have a settings option on the context menu if we haven't implemented channel specific settings.

Finally, it would be good if the context menu for users had a "mention" option.

If the first happens, I'll give me +1. If the second happens as well, you get bonus points.

@xPaw
Copy link
Member Author

xPaw commented Feb 19, 2016

@YaManicKill is it fine by you if I just comment it out? We will need it eventually.

@AlMcKinlay
Copy link
Member

I'm never a massive fan of adding commented out code, tbh. I'd prefer it if we just removed it entirely until we use it. Feel free to stick it up on a branch so the code doesn't get lost.

@xPaw
Copy link
Member Author

xPaw commented Feb 19, 2016

@YaManicKill Removed.

@AlMcKinlay
Copy link
Member

Awesome. 👍 from me.

@astorije
Copy link
Member

Great job @xPaw :-)

A few comments:

  • I'm still not entirely convinced by the right-click for this, but this is a personal opinion so I can't use this as an argument. What about a dropdown button (using angle-down or caret-down) on the right of the channels instead of a right-click? I can't figure out how to describe that nicely, but think something like Gmail or Slack if you use these:

    screen shot 2016-02-20 at 19 53 23

    Problem with that is that if we might have to display a caret for every channel on mobile :-/
    My point being that right click menus on mobile apps are very rare these days because impractical, and usually on desktop web apps too (although not inexistent). I don't have a better solution at the moment so I'd be OK with this (as long as we put there only redundant actions) but I'm still a bit puzzled.

  • (Discussion) Future packages might want to add entries on this, do you think we will be able to have a nice API for this?

  • Why is #channel clickable? I would just leave "× (Close|Leave|Disconnect)" with no other entry at the moment.

  • The light-blue hovering color does not work very well with the default theme. Could you try to re-use something maybe? Right now I'm thinking either the grays behind the nick in the message input (#f6f6f6) or the user filter (#fafafa), or the green of the connect button (#84ce88), ... What do you think? (If you have screenshots of your tries, that'd be great :-))

@xPaw
Copy link
Member Author

xPaw commented Feb 21, 2016

What about a dropdown button (using angle-down or caret-down) on the right of the channels instead of a right-click?

Have fun clicking a small arrow on a mobile device, holding down on a channel name is much more intuitive and doesn't require tapping into pixels perfectly.

(Discussion) Future packages might want to add entries on this, do you think we will be able to have a nice API for this?

I don't see why not.

Why is #channel clickable? I would just leave "× (Close|Leave|Disconnect)" with no other entry at the moment.

Displaying a "heading" with channel/user name in context menu is helpful to see on which entry you clicked on. And making it clickable to bring it into focus is just a bonus. (e.g. username opens a query, and server brings it into focus). Plus, if we ever add context menus to channel names in parsed messages, we can add Join in there.

The light-blue hovering color does not work very well with the default theme.

Will take a look. EDIT: #f6f6f6 works absolutely fine, changed it to that.

@xPaw
Copy link
Member Author

xPaw commented Feb 21, 2016

I've rebased the PR and fixed a bug where menu could go off screen due to incorrect position calculation.

@AlMcKinlay
Copy link
Member

What about a dropdown button (using angle-down or caret-down) on the right of the channels instead of a right-click?

Have fun clicking a small arrow on a mobile device, holding down on a channel name is much more intuitive and doesn't require tapping into pixels perfectly.

Yeah, I agree with @xPaw on this one.

Why is #channel clickable? I would just leave "× (Close|Leave|Disconnect)" with no other entry at the moment.

Displaying a "heading" with channel/user name in context menu is helpful to see on which entry you clicked on. And making it clickable to bring it into focus is just a bonus. (e.g. username opens a query, and server brings it into focus). Plus, if we ever add context menus to channel names in parsed messages, we can add Join in there.

Yeah, I think this is reasonably self-explanitory what would happen. If you are in the room, take you to the room. If you aren't in the room (when we can click on room names in chat) then join the room and take you there. Makes sense to me.

@astorije astorije assigned AlMcKinlay and unassigned AlMcKinlay Feb 25, 2016
@maxpoulin64
Copy link
Member

👍 I like it and am looking forward to a bunch of the actions that could be added to it, especially on mobile. It will just need to be rebased for #94's new templates (and maybe #91?)

I also agree with @xPaw and @YaManicKill, especially on mobile. I hate having to aim for a small menu when I can just right click anywhere, and tapping small things is a nightmare especially when you want to do it quickly. Maybe when there's more options in the menu we could replace the [x] by the icon as @astorije suggested but keep the right click? We could also replace the [Leave][Close] button in channel windows by a [Menu] that opens the dropdown as well, so the menu is easy to spot.

@astorije
Copy link
Member

Re: that hard to click on menu icon, I agree with you. On mobile, it's impractical.

I am ready to bend to the general will here (as long as context menu actions are available somewhere else!!) but I am still thrown off a lot by the right click menu (and context menu in general, but that's different).
Among all the web apps I have used to far, I am yet to find one that does right click menus well. Heck, even Gmail's right click menus suck, as they are very inconsistent with each other. But that's ok as they always duplicate actions available somewhere else on the page.
Also, of all the apps I use on mobile, I can't think of one that has a right click / long click menu (not saying there are none out there, just my personal use).

Modern mobile UX simply do not rely on right click (well, long click) menus as they are hidden (i.e. not intuitive at all) and hard to reach (have to wait 1 second or so every time, have to make sure you click on the right item, ...).

I'm OK going for this as is at the moment, but still feel like here right click menu is a cheap solution to a real UX concern. I do believe we should be more considerate of global UX rather than throwing everything we don't know where to put in the context menu. But by adding my requirement of never having something that is not available somewhere else (which is very common when you think about it), I guess this is addressed.
Note that it also has a small negative effect, as it disables the native browser right click, which can contain entries based on browser extensions among other things.
In terms of UI, I am not too thrilled about it, but the change in the background helps.

So here you are, I believe I have shared all my UX concerns and conditions :-)
As long as the right click is not essential to someone's use of The Lounge, and that we are not set in stone in leaving it alive as-is if it creates more issues than it solves in the future, I guess I'm 👍.
At some point, I'll poke some UX experts to have their opinion on the app altogether, we'll see if they are thrown off by this or something else.

@maxpoulin64
Copy link
Member

@astorije: Yeah, I would definitely keep those options available elsewhere as well. This is why I proposed to duplicate them with the [Leave] button in a channel and so on. So it's really just another way or a shortcut to do actions from the sidebar.

As for the menus on user names, I guess we could open the menu on click. This is where I assume we will add the most options in the future (such as op, voice, kick, ban, kickban, mention, query, whois). I will be really happy when I will be able to make a PR to add a menu item to have the selected nick added to the input field! Right now highlighting someone whose name is not trivial is horrible.

@xPaw
Copy link
Member Author

xPaw commented Feb 28, 2016

Opening menu when clicking on names isn't such a bad idea, probably much better than automatically executing whois.

In fact, we could be displaying whois information in context menu for the user, something like HexChat does it:
hexchat_2016-02-28_13-44-29

But all of this is for another time, I would really like for this to be merged in first.

@AlMcKinlay
Copy link
Member

I liked the idea of opening menu when clicking on names, but I'm sure when I suggested that a while ago, someone didn't like it. Was it @astorije ?

Other than that, I'm totally 👍 on this as it is just now, and we can improve it over time. It's a reasonably big change and it's a great feature that we don't want to wait too long to get it in.

@maxpoulin64
Copy link
Member

This PR already has multiple 👍, merging.

Lets just keep in mind to keep the buttons as well for touch-friendliness.

maxpoulin64 added a commit that referenced this pull request Mar 6, 2016
@maxpoulin64 maxpoulin64 merged commit 94bcb21 into thelounge:master Mar 6, 2016
@AlMcKinlay
Copy link
Member

Wooo. Can we remove the hover close button now? 😛

@maxpoulin64
Copy link
Member

@YaManicKill No, not yet. @astorije still has some accessibility concerns about this one and it was merged semi-accidentally. Plus the close button is kinda useful when I do channel cleanup and close a bunch at once. So far there is no reason to replace it so I think it should stay there for now. We should agree on accessibility concerns before putting things only available through context menus. We'll remove/replace it once those issues are solved. But at least we have them so we can add some features to it, such as OP shortcuts that are already available through commands.

@AlMcKinlay
Copy link
Member

Well the reason is that having the close buttons there can very easy lead
to accidental closing of channels. I do it ever few weeks and I've been
using the app for ages now, and several others have said it has been an
issue for them.

I'd be fine with a different solution than that, but there definitely is a
reason to look into it. Its confusing having something only appear when
hovering and can lead to issues.

On Sun, 6 Mar 2016, 07:31 Max-P, notifications@github.com wrote:

@YaManicKill https://github.com/YaManicKill No, not yet. @astorije
https://github.com/astorije still has some accessibility concerns about
this one and it was merged semi-accidentally. Plus the close button is
kinda useful when I do channel cleanup and close a bunch at once. So far
there is no reason to replace it so I think it should stay there for now.
We should agree on accessibility concerns before putting things only
available through context menus. We'll remove/replace it once those issues
are solved. But at least we have them so we can add some features to it,
such as OP shortcuts that are already available through commands.


Reply to this email directly or view it on GitHub
#9 (comment).

@xPaw xPaw deleted the contextmenu branch March 7, 2016 17:17
@astorije astorije added this to the 1.4.0 milestone Oct 8, 2016
@AlMcKinlay AlMcKinlay removed their assignment Mar 12, 2018
brunnre8 pushed a commit to brunnre8/thelounge that referenced this pull request Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants