Add generic search engine menu item #164
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Squashed commits: [WIP] Simpler implementation of adding generic "Search with ..." menu item (search engine title and url provided by user). Still needs some tweaking [WIP] Ensure URLs copied from Chrome's available search engines work with this implementation [WIP] Update documentation, renaming option apply fixes indicated by XO Merge branch 'sindresorhus:main' into add-other-search-engine wording tweaks, clean ups to be more consistent simplify more tweaks more tweaks reword use tabs instead of spaces for example (shows up in readme with refined github) properly handle case when addSearchWithOther option is not set add a "control" test where no options are set to ensure default scenario is working Revert "add a "control" test where no options are set to ensure default scenario is working" This reverts commit 39d8512. fix casing more doc tweaks missed rewording simplify @Property items * move length check of addSearchWithOther closer to undefined check (confused when revisiting after awhile) * update ts types file to match readme's property section * fix whitespaces more tweaks fix XO trailing space errors add types for addSearchWithOther properties
hns258
force-pushed
the
add-other-search-engine-squashed
branch
from
6eeade7
to
53a6837
Compare
I don't think that's needed.
I don't think validation is needed. |
sindresorhus
requested changes
Sep 22, 2022
hns258
force-pushed
the
add-other-search-engine-squashed
branch
from
0e0c4d6
to
7d9280f
Compare
hns258
force-pushed
the
add-other-search-engine-squashed
branch
from
18f7d07
to
e934d0e
Compare
|
Realized this PR probably isn't the preferred solution to the referenced issue, closing it. Apologies for any wasted time. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adding an
addSearchWithOtheroption that displays aSearch with {searchEngine}menu item when an object containing thetitleandurlof a search engine is provided.Just wanted to mention a few items I wasn't sure about (still relatively new to js):
addSearchWithOtherexpects but wasn't sure if that'd be "overkill" since a developer has to jump to another section to see the properties needed. I can update to one of those if preferred, though.showSearchWithOther,showOtherSearchEngine, or anything else. I just wasn't sure if there might be confusion with the others starting withshow...being booleans (but maybe it's clear enough with the readme and types file?) so for now decided to err on "more clarity" despite looking a bit out of place.urlprovided but due to the following, it seemed safe to assume that for ~95% of the use cases, theurlwould be "clean" so decided not to but can update, if preferred.Fixes #155