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

InteractionID & Count Explainer #133

Merged
merged 5 commits into from
Oct 31, 2023
Merged

InteractionID & Count Explainer #133

merged 5 commits into from
Oct 31, 2023

Conversation

zuoaoyuan
Copy link
Contributor

@zuoaoyuan zuoaoyuan commented Apr 12, 2023

Add an explainer for interactionId & Count.

@zuoaoyuan zuoaoyuan requested a review from mmocny April 12, 2023 16:33
@zuoaoyuan zuoaoyuan self-assigned this Apr 12, 2023
@mmocny
Copy link
Contributor

mmocny commented Oct 26, 2023

I like the latest explainer. I made some updates directly to your readme. I tried to submit as a PR so you can see a nice diff, but there was an error and I was able to just push a change directly into your branch. You can see the diff.

LGTM!

@mmocny
Copy link
Contributor

mmocny commented Oct 26, 2023

Regarding the CI build issue, its based on the actual spec build, not this readme.

See this error:
Screenshot 2023-10-26 at 16 07 57

We may want to try to fix that.

@mmocny
Copy link
Contributor

mmocny commented Oct 26, 2023

Here is the line that is breaking the build: 7fa8ac5

Indeed, it seems to have a missing attribute value. I will file a separate spec issue.

@zuoaoyuan
Copy link
Contributor Author

@mmocny The updates looks great! And thanks for digging into the build issue.

Copy link
Contributor

@mmocny mmocny left a comment

Choose a reason for hiding this comment

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

LGTM with the one remaining comment at the bottom, to update the code snippet just a bit.

interactionID-explainer.md Outdated Show resolved Hide resolved
interactionID-explainer.md Outdated Show resolved Hide resolved
interactionID-explainer.md Outdated Show resolved Hide resolved
interactionID-explainer.md Outdated Show resolved Hide resolved
interactionID-explainer.md Outdated Show resolved Hide resolved
interactionID-explainer.md Show resolved Hide resolved
@mmocny mmocny merged commit ccdfd6b into w3c:main Oct 31, 2023
1 of 3 checks passed
@mmocny
Copy link
Contributor

mmocny commented Oct 31, 2023

I merged this in, but it didn't automatically squash the commits. I'm so accustomed to this being the default option, that I forgot to look for it. (It just means we have a few extra commits in git history, I wouldn't bother re-writting main branch to fix this).

I think we can change this in the github project settings for next time.

@mmocny
Copy link
Contributor

mmocny commented Oct 31, 2023

Hmm, I don't have access to settings for the repo, and it may even be explicitly disabled as an option for merge... Some docs references:

How to squash merge
Configure repo for squashing prs

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