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: chrome.sidePanel POC 2 #7266

Closed
wants to merge 33 commits into from
Closed

#7228: chrome.sidePanel POC 2 #7266

wants to merge 33 commits into from

Conversation

fregante
Copy link
Contributor

@fregante fregante commented Jan 4, 2024

Forked from #7232

I was working on an alternative solution here and I'm posting it here so that we can determine which behavior we prefer. Here for example:

  • there's no popup anymore; the button opens the sidebar, always
  • the sidebar closes when switching to a tab that doesn't have (matches the MV2 behavior)
Screen.Recording.1.mov

@fregante
Copy link
Contributor Author

fregante commented Jan 4, 2024

Please don't judge the code yet, I'm just going through every part that fails and it has not been reviewed.

Report

First successful run, however the current sequence is:

  1. Open sidebar
  2. Reload tab
Screenshot 2

Also first successful auto-open and update from the page editor:

Screen.Recording.2.mov

@fregante
Copy link
Contributor Author

fregante commented Jan 4, 2024

The sidebar bricks also already work:

Screen.Recording.3.mov

@grahamlangford

This comment was marked as resolved.

@fregante

This comment was marked as resolved.


if (!isEmpty(activateOptions)) {
const seqNum = renderSequenceNumber;
renderSequenceNumber++;
Copy link
Contributor Author

@fregante fregante Jan 8, 2024

Choose a reason for hiding this comment

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

What happens when this value is lost after a page reload? The sidebar will stop updating until a higher number is reached.

This is another instance of why the content script can no longer be the sidebar controller.

  • Move/drop sequence number

@grahamlangford
Copy link
Collaborator

If you'd like to review/test, it'd be great to see what's still broken, ideally accompanied by repros. I fixed everything I noticed wrong so far, or added some TODOs inline and in this review.

I'm reviewing your changes now. I'll test it thoroughly today and let you know what I find.

If this instruction has since changed, we can think about merging these changes with the MV2-compatible API.

Unless the complexity of maintaining MV2 + old sidebar alongside MV3 + side panel is high, we'll be keeping both together. So considering how to pull this in in a backwards compatible way makes sense as a next step.

@fregante fregante added the mv3 label Jan 8, 2024
Co-authored-by: Graham Langford <30706330+grahamlangford@users.noreply.github.com>
@fregante
Copy link
Contributor Author

fregante commented Jan 11, 2024

Squashing and reopening into another PR and tracking issue:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants