-
Notifications
You must be signed in to change notification settings - Fork 296
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
Add Firefox Social API to the tools. Fixes #101. #213
Conversation
Nice, I like that :) Its working but i have some suggestions: |
I don't think I have a say in whether or not the sidebar shows, the social API is mostly configuration. I didn't know about custom favicons, there's no evidence of that in the code. |
This part is done. Is there anywhere to look for custom favicons ? |
After a discussion with @alexisju, no more sidebar (it's probably useless), and an experiment at hiding the header and footer for that popup. I personally like it. |
works well! |
Something to mention is that commenting the footer removed the part where the delete confirmation happened. The fact is that if I leave that popup on, it steals the focus from the share popup and clicking yes or no doesn't make a difference, the delete won't happen. So if we want to keep that feature in that popup, it has to be a pure html/js confirmation. |
You missed a few spot for closing the bookmarklet in You can checkout loginform.phtml from my PR #216 It's a bit nicer when the user is not logged in yet. And I would use |
Should be good now. Why did you add a few |
It was to size the panel properly but it's not critical. https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Social_API/Share#How_to_properly_size_your_share_panel |
Ready to merge (?) |
Works fine, thanks! |
As discussed in #101, here comes the PR.
The wording of the button might be debatable, I didn't know how to call it, you might have better ideas than I do.
I didn't do any browser detection either since that would make this patch a bit more complex, I'm not sure it's worth it since it doesn't break other browsers.