Skip to content
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

#7228: Add support for sidePanel in the MV3 build #7354

Merged
merged 37 commits into from
Jan 24, 2024
Merged

Conversation

fregante
Copy link
Contributor

@fregante fregante commented Jan 17, 2024

The MV3 sidePanel PR seems stable and working, so the setup seems to be correct.

In this PR I will restore MV2 support on top of the new sidePanel setup.

Next steps

  • Create list of actions that should be successful.
    Graham is preparing: https://www.notion.so/pixiebrix/MV3-Side-Panel-Regression-Tests-12495993574541afa987e6d3f9762b67
    Plus:
    • "Show sidebar brick"
    • "Hide sidebar brick"
    • Sidebar opens on browser action click
    • Temporary forms open the sidebar and close it afterwards (if there are no other panels)
    • Side panels appear in the sidebar
    • The side panels are created from the page editor
    • The side panels are updated from the page editor
    • The side panels are dropped after being deleted from the page editor
  • Verify that the previous progress was not lost
  • Verify that the MV2 version still works

What's not part of this PR

Copy link

codecov bot commented Jan 18, 2024

Codecov Report

Attention: 191 lines in your changes are missing coverage. Please review.

Comparison is base (8e850bb) 72.58% compared to head (d24f778) 72.52%.

Files Patch % Lines
src/contentScript/sidebarController.tsx 53.42% 34 Missing ⚠️
src/sidebar/sidePanel/messenger/api.ts 23.68% 29 Missing ⚠️
src/sidebar/hooks/useConnectedTargetUrl.tsx 29.62% 19 Missing ⚠️
src/background/browserAction.ts 0.00% 18 Missing ⚠️
src/background/sidePanel.ts 0.00% 11 Missing ⚠️
src/sidebar/connectedTarget.tsx 50.00% 11 Missing ⚠️
src/tinyPages/RestrictedUrlPopupApp.tsx 43.75% 9 Missing ⚠️
src/bricks/effects/sidebar.ts 33.33% 8 Missing ⚠️
src/sidebar/SidebarErrorBoundary.tsx 16.66% 5 Missing ⚠️
src/sidebar/protocol.tsx 37.50% 5 Missing ⚠️
... and 23 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7354      +/-   ##
==========================================
- Coverage   72.58%   72.52%   -0.07%     
==========================================
  Files        1204     1211       +7     
  Lines       37650    37827     +177     
  Branches     7060     7101      +41     
==========================================
+ Hits        27330    27433     +103     
- Misses      10320    10394      +74     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor Author

@fregante fregante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preliminary self-review.

⚠️ Code still untested, but ✅ CI is finally green!

await rehydrateSidebar({
tabId,
});
updateSidebar(contentScriptTarget);
Copy link
Contributor Author

@fregante fregante Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to double-check:

  • Is updateSidebar equivalent to rehydrateSidebar
  • Is updateSidebar equivalent to the optional part of showSidebar?

Comment on lines 39 to 49
<div className="mt-2">
Looking for the Extension Console?{" "}
<a
href="#"
onClick={async () => {
await browser.runtime.openOptionsPage();
}}
>
Open the Extension Console
</a>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this was removed by mistake?

Copy link
Contributor Author

@fregante fregante Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was merged into the other component because it was 70% duplicate code and I had to further customize the on click handler

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restored and verified:

Screenshot 16

Copy link
Collaborator

@grahamlangford grahamlangford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditionally approved on:

  • Extension Console link restored in the restricted browser popup

Copy link

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

@fregante fregante merged commit da99396 into main Jan 24, 2024
13 checks passed
@fregante fregante deleted the F/mv3/sidePanel-4 branch January 24, 2024 10:48
@grahamlangford grahamlangford added this to the 1.8.8 milestone Feb 1, 2024
@fregante fregante removed the do not merge Merging of this PR is blocked by another one label Feb 18, 2024
@fregante fregante mentioned this pull request Mar 27, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment