Skip to content

feat: Add support for theme matching and scroll reporting via postMessage #461

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

Merged
merged 4 commits into from
Sep 21, 2022

Conversation

jackbrewer
Copy link
Contributor

@jackbrewer jackbrewer commented Sep 16, 2022

This PR removes the existing (but unused) functionality for reading a users theme preference via query parameter.

Instead, we will use postMessage to communicate cross-origin between the report and the App. We'll be sending a theme preference in one direction, and sending back a message when the scroll reaches the page bottom.

This approach has less flexibility than before, in that the theme switching now only works within an iframe (which is fine for current needs). On the plus side, the injection does feel cleaner than the previous iteration as we're now appending a standalone script at the end, rather than injecting mid-function.

@jackbrewer jackbrewer requested a review from a team September 16, 2022 23:12
@jackbrewer jackbrewer self-assigned this Sep 16, 2022
@netlify
Copy link

netlify bot commented Sep 16, 2022

👷 Deploy Preview for plugin-lighthouse processing.

Name Link
🔨 Latest commit a99325b
🔍 Latest deploy log https://app.netlify.com/sites/plugin-lighthouse/deploys/63250fa5b004a800081c1efd

Copy link
Contributor

@nasivuela nasivuela left a comment

Choose a reason for hiding this comment

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

LGTM!

@jackbrewer jackbrewer merged commit 42822bc into main Sep 21, 2022
@jackbrewer jackbrewer deleted the jbr/postMessageCommunication branch September 21, 2022 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants