Toggle mode #30
Toggle mode #30
Conversation
Remove calls to message() in the popup. Add dynamically updated title to the Toggle button.
|
Thanks for the PR. I am a bit busy with a few deadlines Will try to have a look at it after next week. |
|
Hey @napsternxg, any chance of you reviewing this PR in the foreseeable future? |
|
Hi,
I have been a bit busy recently. I will review this soon and update the
extension.
…On Mar 21, 2018 8:49 AM, "Yanis Batura" ***@***.***> wrote:
Hey @napsternxg <https://github.com/napsternxg>, any chance of you
reviewing this PR in the foreseeable future?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#30 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAG4JrY4J9tjTOXrd579JEmZvCEDP0Eaks5tgcbIgaJpZM4SGPbE>
.
|
|
Wow, this PR is still there! |
| console.log(`URL Item successfully added.`); | ||
| }; | ||
|
|
||
| var add_exists = function(urlItem) { |
There was a problem hiding this comment.
I think this feature is a bit misleading given the original implementation showed error message when you tried to add existing URL. I can think of the following workarounds:
- Ask for permission if trying to remove page which is already added.
- Change the badge of ReadLater logo to show that the current page is already present. Add a notification for users when the app is updated.
The main idea is that user's should not loose existing URLs when we make new functionality available in the UI.
There was a problem hiding this comment.
Actually, what this PR does is implement the feature you approved in #28.
Changing this logic will defeat its main purpose.
There was a problem hiding this comment.
Sorry, I forgot about the details.
Is it possible to rename the button to ADD and REMOVE based on the presence of the URL in the storage?
This way the UI will make it more explicit to the user that the URL will be removed.
Also, if the invocation is by keyboard shortcut then we should ask for user confirmation before item removal.
I am trying to ensure this as many users have contacted me about missed URLs because the app instructions were not clear on some parts. It is important to make the usage of the app as simple and explanatory as possible.
There was a problem hiding this comment.
I think there's no point to discuss this any more.
Thanks for your time!
| var add_exists = function(urlItem) { | ||
| // Tried to add but it exists --- then remove | ||
| console.log("The current page is already present -- trying to remove...") | ||
| removeUrlHandler(urlItem.url); |
There was a problem hiding this comment.
This is the line which should be altered with some decision rule based on the above comment.
|
|
||
| let urlItem = createNewURLItemFromTab(tab); | ||
| success_callback(urlItem); | ||
| getCurrentTab().then(tab => { |
There was a problem hiding this comment.
I like the use of Promise, never used it before in any real project. Thanks. May do a future refactor to convert all callbacks to promises.
| @@ -21,7 +21,7 @@ | |||
| <div id="header"><h1>ReadLater</h1><span>Save now, Read Later</span></div> | |||
| <!--<div id="currentLink"></div>--> | |||
| <div id="container"> | |||
| <span id="addBtn">ADD</span> | |||
| <span id="toggleBtn">TOGGLE</span> | |||
There was a problem hiding this comment.
Is it possible to rename this button to ADD and REMOVE based on the presence of the URL in the storage?
This way the UI will make it more explicit to the user that the URL will be removed.
Also, if the invocation is by keyboard shortcut then we should ask for user confirmation before item removal.
This pull request changes the logic of working with the current URL.
add-urlcommand has been changed totoggle-urlcommand, which now either adds or removes the current URL to/from the link list. This action will also be reported by a short Chrome notification (anotificationspermission was added for that purpose).Also, some minor UI modifications were made: