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 menu to config_item #526

Merged
merged 4 commits into from Nov 9, 2017

Conversation

@facontidavide
Copy link
Contributor

facontidavide commented Nov 6, 2017

Hi,

I found the Remove button very counter intuitive, in particular when the config_item is collapsed.
I hope you will find this PR useful.
image

Regards

Davide

@evenator

This comment has been minimized.

Copy link
Contributor

evenator commented Nov 7, 2017

👍 From me. I've always found that button unintuitive myself.

@facontidavide

This comment has been minimized.

Copy link
Contributor Author

facontidavide commented Nov 7, 2017

Just a personal opinion. If this PR is eventually merged, may be you want to consider removing the Remove button on bottom and stop using orange to highlight the selected config_items.

Up to you ;)

@evenator evenator requested a review from pjreed Nov 7, 2017
@pjreed

This comment has been minimized.

Copy link
Member

pjreed commented Nov 7, 2017

👍

I like this, although I don't think the "Remove" button on bottom should be removed. In general I think that a context menu should never be the only way to access a function, since right-clicking not be available or may not work right on every platform, and it's not easily discoverable for users who don't expect to be able to right-click on everything.

@pjreed
pjreed approved these changes Nov 7, 2017
evenator added 2 commits Nov 7, 2017
@pjreed pjreed merged commit 45207c1 into swri-robotics:kinetic-devel Nov 9, 2017
pjreed added a commit to pjreed/mapviz that referenced this pull request Nov 9, 2017
* add context menu to config_item

* typo fix

* Change pointer syntax to match repository conventions

* Change pointer syntax to match conventions
pjreed added a commit that referenced this pull request Nov 10, 2017
* add context menu to config_item

* typo fix

* Change pointer syntax to match repository conventions

* Change pointer syntax to match conventions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.