Skip to content

Change to using contextmenu plugin - #10

Open
Alpvax wants to merge 5 commits into
Trymunx:masterfrom
Alpvax:master
Open

Change to using contextmenu plugin#10
Alpvax wants to merge 5 commits into
Trymunx:masterfrom
Alpvax:master

Conversation

@Alpvax

@Alpvax Alpvax commented Jan 4, 2019

Copy link
Copy Markdown
Collaborator

U̶n̶a̶b̶l̶e̶ ̶t̶o̶ ̶t̶e̶s̶t̶ ̶o̶n̶ ̶m̶y̶ ̶p̶h̶o̶n̶e̶ ̶b̶e̶f̶o̶r̶e̶ ̶c̶r̶e̶a̶t̶i̶n̶g̶ ̶P̶R̶,̶ ̶s̶o̶ ̶p̶r̶o̶b̶a̶b̶l̶y̶ ̶d̶o̶n̶'̶t̶ ̶m̶e̶r̶g̶e̶ ̶y̶e̶t̶.̶

Ready for merge.

@Alpvax

Alpvax commented Jan 4, 2019

Copy link
Copy Markdown
Collaborator Author

I would recommend squashing and merging (So only the first commit message is listed in the history).

@Trymunx Trymunx left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm happy to merge if:

  • you make it possible for me to use my own class name for context menus. It might be nice if this could be scoped so different context menus could be used, although if it's defined only on the root instance of the app this wouldn't be possible anyway (and it wasn't before).
  • it's definitely possible for me to overwrite the styles with my own ones. It might be nice to keep the styles set on it as default context menu styles, but these should have colours set, not commented out!

@Alpvax

Alpvax commented Jan 5, 2019

Copy link
Copy Markdown
Collaborator Author

What should the default colours be set to?

@Trymunx

Trymunx commented Jan 7, 2019

Copy link
Copy Markdown
Owner

I think default colours should be white and grey. Most context menus tend to do this. You could go for blue instead of grey as the highlight colour, as that's common for focus in browsers, but don't think it looks as nice.

Now displays the DOM element targeted (not the one the listener is
attached to) and the VueComponent that declares that element (the lowest
parent component).
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.

2 participants