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

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
Copy link
Contributor

evenator commented Nov 7, 2017

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

@facontidavide
Copy link
Contributor Author

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 November 7, 2017 15:18
@pjreed
Copy link
Contributor

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 merged commit 45207c1 into swri-robotics:kinetic-devel Nov 9, 2017
pjreed pushed 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants