Add to Pocket from link context menu

#27 Merged at f50473f
Repository
adambro
Branch
add_link_context_menu
Repository
pabuisson
Branch
master
Author
  1. adambro
Reviewers
Description
No description

Comments (6)

  1. Pierre-Adrien Buisson repo owner

    Hey @adambro ! Thanks for this PR, you're the first contributer 🎉

    Your PR seems to work perfectly, thanks ! Do you think you could add a second context menu when the user clicks on the background of the current page (judging from the MDN doc, it should be a page context), so that they can add the current page without needing to move the pointer to the toolbar icon and click twice. I myself use this feature really often when using other browsers and therefore other Pocket addons, and I think it would be a nice addition to what you already did.

  2. adambro author

    Funny fact I've noticed on Firefox Developer Edition (53.x): Save to Pocket is there in menu for right-click on link and on page. The same happens on stable Firefox (v 52), but only when I start with clean profile. Maybe it's time to reboot my profile...

  3. Pierre-Adrien Buisson repo owner

    Yes when I first tested your PR, I thought it was your context menu, and then I realized it was the one embedded with Firefox and Pocket "integration" 😅

    Your PR works well, that' great! When I tested your PR and adding links with the context menus, I realized that IMP was missing a UI indication that something had happened. I've added a badge (using the Pocket blue-green color) showing that a page has been added and then reverting back to the original badge display (items count or nothing).

    So I've made a few commits those last days, can you please rebase your branch on master, check that everything works well and update the PR? Then I'll be able to merge 😃

  4. adambro author

    OK, I've merged master into my branch and tested - everything is working fine. Noticed blue icon when new article has been added - looks cool :) Anything else?

    BTW: why you're using Babel?

  5. Pierre-Adrien Buisson repo owner

    Great! Yeah the "flashing" indicator looks pretty cool :)
    At first I wanted to change the color of the toolbar icon itself but it was much more work, especially to get a smooth transition between the different icon colors.

    Babel... at first, it was mainly to take advantage of ES2015 modules, but I've seen that Webpack 2 now deals with those out of the box. I still keep Babel for the moment as it could allow me to write ES2016 code if I want to, or if I want to develop for other browser, make sure that it compiles to ES5 and that every browser will understand the code – I've had some issues on a website of mine with old Safaris that didn't support let for instance –.

    At the moment, I'm pretty sure that I could drop it though.