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

Use Messenger for sidebar bricks #1642

Merged
merged 3 commits into from Oct 14, 2021
Merged

Use Messenger for sidebar bricks #1642

merged 3 commits into from Oct 14, 2021

Conversation

fregante
Copy link
Collaborator

@fregante fregante commented Oct 13, 2021

Here I replaced sendMessage-based handlers (not liftBackground)

Pretty straightforward, except that it made me question a few things:

Particularly around something I have not touched yet: forwardWhenReady/RENDER_PANELS_MESSAGE

Comment on lines +49 to +55
registerActionFrame: getMethod("REGISTER_ACTION_FRAME"),
forwardFrameNotification: getMethod("FORWARD_FRAME_NOTIFICATION", {
isNotification: true,
}),
showActionFrame: getMethod("SHOW_ACTION_FRAME"),
hideActionFrame: getMethod("HIDE_ACTION_FRAME"),
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Test report

Working

  • REGISTER_ACTION_FRAME
  • SHOW_ACTION_FRAME
  • HIDE_ACTION_FRAME

Untested

  • FORWARD_FRAME_NOTIFICATION

Can you tell me what I should set up to test this? I have not ported it messenger yet because a couple of key components might be missing

Copy link
Contributor

Choose a reason for hiding this comment

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

FORWARD_FRAME_NOTIFICATION is used to pass information from the content script on the main page to the sidebar panel. It's used in many of the methods in actionPanel/native.tsx via calls to renderPanels

The way to test would be:

If the messages aren't working you won't see the header for the panel and won't see any panel content on pages where the panel should appear

@fregante fregante changed the title WIP: Use Messenger for browser action Use Messenger for browser action Oct 13, 2021
@fregante fregante marked this pull request as ready for review October 13, 2021 23:07
@fregante fregante changed the title Use Messenger for browser action Use Messenger for browser action bricks Oct 13, 2021
@fregante fregante changed the title Use Messenger for browser action bricks Use Messenger for sidebar bricks Oct 13, 2021
Copy link
Contributor

@twschiller twschiller left a comment

Choose a reason for hiding this comment

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

Good to merge once we switch the effect method definitions back to keep the stack trace/for consitency

@fregante
Copy link
Collaborator Author

Test report update

Working

  • FORWARD_FRAME_NOTIFICATION

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

Successfully merging this pull request may close these issues.

None yet

2 participants