Improve lookUpSelection implementation
#76
Conversation
caesar
changed the title
lookUpWord implementation
caesar
changed the title
lookUpWord implementationlookUpSelection implementation
What other places? I don't really like making things flexible if it has no purpose. |
Well, for example, to implement a "Search Google for {selection}" menu item, or if someone wanted to say change the "Copy" menu item to "Copy {selection}".
In principle, I agree. However, in this case, the "generic" implementation is simpler and more maintainable than a "special-case" implementation would be, in my opinion. A special-case implementation would still have to be implemented at the same point in the code (after applying custom titles), so while looping through the menu items it would have to check each one to see if it had the appropriate |
* master: Add Services submenu on macOS (#73) # Conflicts: # index.d.ts # index.js
Good point. You should include that in the docs to make it clearer what the user could use it for. |
readme.md
Outdated
| Overwrite labels for the default menu items. Useful for i18n. | ||
| Override labels for the default menu items. Useful for i18n. | ||
|
|
||
| The placehlder `{selection}` may be used in any label, and will be replaced by the currently selected text. |
There was a problem hiding this comment.
This needs to be more comprehensive. It needs to mention what the user has to check for and how to guard against selection not being available (like we do in the built-in menu. Maybe show an example of a custom menu using {selection}?
| The placehlder `{selection}` may be used in any label, and will be replaced by the currently selected text. | |
| The placeholder `{selection}` may be used in any label, and will be replaced by the currently selected text. |
|
@caesar Would you be able to finish this so we can get out a new release? :) |
|
Hey Sindre, sorry for not getting back onto this, I've been really busy. I'm going to be offline for a couple of days, but I'll see if I can take a look at this right now to get it finished before I leave so you can merge. I think we're only talking about documentation improvements now, right? |
|
Correct |
|
I think that's quite comprehensive, what do you think? |
|
Looks great 👌 |
|
Thanks Sindre 👍 |
Further to #71, and per discussion there and in #73 (comment):
Change the order of menu actions in code to match the default menu order.
Note, as well as
lookUpSelectionI have movedinspect, which was also in the wrong place in the order.Add
lookUpSelectiontoLabelsinterface inindex.d.ts,with note on usage (specifically, that if a static label is set, it will no longer be dynamic).Also add a similar but more detailed note to
readme.md.[update] Implement
{selection}placeholder for menu titles to enable full localisation of theLook Up “{selection}”item.This commit will probably conflict with #73; if so, I will fix that one once this is committed.