Skip to content
This repository has been archived by the owner on Jul 29, 2024. It is now read-only.

fixes issue #284, use new cross-browser scripting api for registering… #301

Merged
merged 21 commits into from
Aug 4, 2022

Conversation

rhelmer
Copy link
Contributor

@rhelmer rhelmer commented Jul 16, 2022

… content scripts

Review expectations: this has integration tests for Firefox (mv2) and Chrome (mv3), so I think just a visual check is OK. If you'd like to try loading this into an extension or load the test extension into your browser that's fine too (but probably redundant with the integration test).

@rhelmer rhelmer self-assigned this Jul 16, 2022
@rhelmer rhelmer marked this pull request as draft July 16, 2022 04:31
Copy link

@aaga aaga left a comment

Choose a reason for hiding this comment

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

The meat of it looks good! A couple functionality questions, and a couple small style things.

src/pageManager.js Outdated Show resolved Hide resolved
src/pageNavigation.js Show resolved Hide resolved
src/pageNavigation.js Show resolved Hide resolved
src/permissions.js Outdated Show resolved Hide resolved
src/pageNavigation.js Show resolved Hide resolved
tests/integration/extension/src/background.js Show resolved Hide resolved
@aaga
Copy link

aaga commented Aug 2, 2022

Another thing we should address: it might be worth it to add a "Chrome/MV3 support" column to that modules table in the README? since it seems we'll be updating these modules piecemeal as and when we need them. Doesn't necessarily need to be in the scope of this PR, but just wanted to put it on the radar as something that would be useful to consumers of this library (primarily future us!).

@rhelmer
Copy link
Contributor Author

rhelmer commented Aug 3, 2022

Another thing we should address: it might be worth it to add a "Chrome/MV3 support" column to that modules table in the README? since it seems we'll be updating these modules piecemeal as and when we need them. Doesn't necessarily need to be in the scope of this PR, but just wanted to put it on the radar as something that would be useful to consumers of this library (primarily future us!).

Good idea, did this as well... I made it a single "Browser Support" column with the text "Firefox" and/or "Chrome", we could use browser icons later if we add more and need to save space, but this seemed simpler and more readable.

@rhelmer rhelmer requested a review from aaga August 3, 2022 00:57
src/linkExposure.js Outdated Show resolved Hide resolved
src/pageTransition.js Outdated Show resolved Hide resolved
Copy link

@aaga aaga left a comment

Choose a reason for hiding this comment

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

let's gamble, try merging! jk

@rhelmer rhelmer merged commit 49d43bc into main Aug 4, 2022
@rhelmer rhelmer deleted the switch-to-scripting-api branch August 4, 2022 00:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants