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

Convert createWindow parameters to options bag and add testid option #2765

Merged
merged 3 commits into from
Sep 26, 2024

Conversation

sirtimid
Copy link
Contributor

@sirtimid sirtimid commented Sep 26, 2024

This PR refactors the createWindow function to use an options bag for its parameters, improving clarity and extensibility. Additionally, it introduces a new testId option, allowing customization of the data-testid attribute for the generated iframe.

We need this change in the MetaMask/ocap-kernel package (see issue), where the createWindow function is used, to set a different testId that doesn't conflict with the snaps iframe. This will ensure clear differentiation when using iframes across different packages.

Updated and expanded the test suite to cover the presence and absence of sandbox and testId attributes.

@sirtimid sirtimid marked this pull request as ready for review September 26, 2024 15:11
@sirtimid sirtimid requested a review from a team as a code owner September 26, 2024 15:11
Copy link

codecov bot commented Sep 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.38%. Comparing base (7d15013) to head (d2828ba).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2765   +/-   ##
=======================================
  Coverage   94.38%   94.38%           
=======================================
  Files         478      478           
  Lines       10208    10209    +1     
  Branches     1557     1558    +1     
=======================================
+ Hits         9635     9636    +1     
  Misses        573      573           

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

Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

LGTM! I technically have the authority to approve this, but we'll wait for approval from someone on the Snaps team.

@sirtimid sirtimid requested a review from a team September 26, 2024 16:59
@sirtimid sirtimid merged commit 60bd1d8 into main Sep 26, 2024
165 checks passed
@sirtimid sirtimid deleted the sirtimid/create-window branch September 26, 2024 17:22
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.

3 participants