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

Spec skeleton for interacting with the fenced frame config mapping #616

Merged
merged 24 commits into from
Jun 28, 2023

Conversation

gtanzer
Copy link
Contributor

@gtanzer gtanzer commented Jun 8, 2023

@gtanzer gtanzer changed the title Spec pending config construction WIP: Spec pending config construction Jun 8, 2023
@JensenPaul JensenPaul added the spec Relates to the spec label Jun 9, 2023
@gtanzer gtanzer changed the title WIP: Spec pending config construction Spec skeleton for interacting with the fenced frame config mapping Jun 9, 2023
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@domfarolino domfarolino self-requested a review June 20, 2023 15:44
Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

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

There are a number of TODOs here which are hard to evaluate. It would be good to know how many of them are going to be resolved in this PR vs after.

Separately: will there be a follow-up to #593 (review) where we actually resolve the promise with a web-exposed FencedFrameConfig object that internally references the config struct?

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

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

OK it sounds like there is some pending work in this PR so let me know when I should take another look next!

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Show resolved Hide resolved
@domfarolino
Copy link
Collaborator

I'll try closing and re-opening this to kick off the PR preview thing again, see if it is "fixed" now.

@domfarolino domfarolino reopened this Jun 28, 2023
Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

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

Pretty much lgtm with a few pretty small things!

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Show resolved Hide resolved
spec.bs Show resolved Hide resolved
@miketaylr miketaylr merged commit e332640 into WICG:main Jun 28, 2023
2 checks passed
github-actions bot added a commit that referenced this pull request Jun 28, 2023
)

SHA: e332640
Reason: push, by miketaylr

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Relates to the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants