Add alert for failed sidebar injection in extension console #4208
Conversation
There was a problem hiding this comment.
Double-clicking/clicking the toolbar icon n times will produce n alerts displayed back-to-back after closing them
Not preventable, but with regular notifications this isn't a big issue anymore.
Why is there so much whitespace above the alert title?
I think their UI expects an icon to appear but it doesn't actually pick it up. Also unfixable (other than displaying our own UI or just using notifications)
Co-authored-by: Federico Brigante <[email protected]>
Co-authored-by: Federico Brigante <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #4208 +/- ##
==========================================
- Coverage 48.46% 48.44% -0.03%
==========================================
Files 876 878 +2
Lines 26073 26085 +12
Branches 5376 5379 +3
==========================================
Hits 12636 12636
- Misses 12518 12530 +12
Partials 919 919
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
src/background/browserAction.ts
Outdated
| if (url.startsWith(optionsPage)) { | ||
| const keyboardShortcut = isMac() ? "Cmd+Opt+C" : "Ctrl+Shift+C"; | ||
| notify.info( | ||
| { tabId: tab.id, page: "any" }, |
There was a problem hiding this comment.
@fregante Is "any" ok here? Ran into an issue where the message would get "forwarded" if page was not specified.
There was a problem hiding this comment.
Try page: "/options.html" but it should work even without it 🤔
"Any" broadcasts the message to all chrome-extension:// pages and the tabId is currently ignored. This means even the background page will get the message and throw an error.
There was a problem hiding this comment.
Indeed we never received tabId-only messages outside the content script so it's not supported:
We still need to change this or else tabId is ignored. I tried this and it works:
| { tabId: tab.id, page: "any" }, | |
| { tabId: tab.id, page: "/options.html" }, |
src/background/browserAction.ts
Outdated
| `PixieBrix Tip 💜\n If you want to create a new blueprint, first navigate to the page you want to modify, then open PixieBrix in the DevTools (${keyboardShortcut}).` | ||
| ); | ||
| } | ||
|
|
||
| // The URL might not be available in certain circumstances. This silences these | ||
| // cases and just treats them as "not allowed on this page" | ||
| await toggleSidebar(tab.id, String(tab.url)); |
There was a problem hiding this comment.
This comment should be moved to the first line of the function + url can be used here.
| { tabId: tab.id, page: "any" }, | ||
| `PixieBrix Tip 💜\n If you want to create a new blueprint, first navigate to the page you want to modify, then open PixieBrix in the DevTools (${keyboardShortcut}).` | ||
| ); | ||
| } |
There was a problem hiding this comment.
We need a return here because no further action is necessary
|
What does this PR do?
Discussion
Outstanding issues:
Demo
Screen.Recording.2022-09-02.at.3.22.41.PM.mov
Checklist