-
Notifications
You must be signed in to change notification settings - Fork 3
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
Added optional delimiter field, extended documentation and renamed plugin, as it is not only about the hostname anymore #4
base: master
Are you sure you want to change the base?
Conversation
Tormen
commented
Oct 3, 2017
- Added optional delimiter field
- Extended documentation (README, options.html, Manifest)
- Renamed plugin, as it is not (only) about the hostname anymore
* Added optional delimiter field * Added README
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there, thanks for contributing this!
It looks good to me, except that you're changing the manifest.json file with a new name, which I think may cause it to be rejected as an update to the old addon.
Can you take a look at the comments and make those changes? Other than that, it's looking great.
protocol = (protocolEnabled ? `${window.location.protocol}//` : ""); | ||
path = (pathEnabled ? window.location.pathname : ""); | ||
return ` - ${protocol}${window.location.hostname}${path}`; | ||
return `${delimiter}${protocol}${window.location.hostname}${path}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep the spaces around ${delimiter}
so users only have to input the characters they want as the delimiter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, disregard this one. I see the use-cases where delimiters wouldn't need spaces.
@@ -1,15 +1,15 @@ | |||
{ | |||
|
|||
"manifest_version": 2, | |||
"name": "Hostname in title", | |||
"name": "URL (protocol, hostname, path) in title", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep this as "hostname in title"