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

More HTML decoding #4146

Closed
wants to merge 6 commits into from
Closed

More HTML decoding #4146

wants to merge 6 commits into from

Conversation

edent
Copy link
Contributor

@edent edent commented Oct 30, 2018

Re #3683

Would it be better if I did each of these commits as a separate PR? There will be quite a lot of them.


/acknowledgements.html ( diff )
/embedded-content-other.html ( diff )
/form-elements.html ( diff )
/images.html ( diff )
/text-level-semantics.html ( diff )

source Outdated
@@ -121425,10 +121425,11 @@ INSERT INTERFACES HERE
Takashi Toyoshima,
Takayoshi Kochi,
Takeshi Yoshino,
<span data-x="" lang="tr">Tantek &Ccedil;elik</span>,
<span lang="tr">Tantek Çelik</span>,
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the data-x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My beautifier identified it as empty when I cleaned that link. Is data-x used for something specific when building the document?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's for cross-references, so in this case it's explicitly marking the span as not being a cross-reference.

@annevk
Copy link
Member

annevk commented Oct 30, 2018

I think grouping them is fine, but some might need some discussion. E.g., −1 is hard to distinguish from -1 so I'm not sure if we should do that as it might lead to folks using -1 instead. (I've used &minus; in other standards for this reason.)

@edent
Copy link
Contributor Author

edent commented Oct 30, 2018

What's the risk of someone using -1 rather than −1?

Would it be better to use text like "negative 1" or "minus 1"?

Happy to make the change.

@annevk
Copy link
Member

annevk commented Oct 31, 2018

I'm mainly worried about introducing inconsistencies in formatting. I don't think switching to text helps as we also format equations with this character. Keeping the existing entity or switching it to &minus; seems good to me.

@edent
Copy link
Contributor Author

edent commented Oct 31, 2018

@annevk ok - reverted to &minus;

@zcorpan ok - reverted the data-x=""

Let me know if this PR is OK to merge and I'll continue with the rest of the encoded entities.

@annevk
Copy link
Member

annevk commented Oct 31, 2018

Thanks, I think this can be merged after rebasing (feel free to force push after fixing). Not entirely clear why it conflicts.

@zcorpan
Copy link
Member

zcorpan commented Oct 31, 2018

I think the conflict is the first commit which is already in master.

@domenic
Copy link
Member

domenic commented Oct 31, 2018

I'd prefer to have only one (or I guess two since we did #4139 already) commit for all these changes, to keep the commit log clean. So it'd be ideal to not merge this until the whole spec is converted and merging would close #3683.

Edit: maybe that is already the case?

@domenic
Copy link
Member

domenic commented Nov 9, 2018

@edent sorry we dropped this for a while. Do you think this is all the decoding work there is to do? If so, we can take care of the rebasing for you, since it's our fault for letting this sit.

@edent
Copy link
Contributor Author

edent commented Nov 9, 2018

@domenic please can you rebase - I'm busy for the next week or so. Then I'll do some more decoding.

@annevk
Copy link
Member

annevk commented Nov 12, 2018

Rebased.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Rebased again, and let's merge this :). Sorry again for the delay.

@domenic
Copy link
Member

domenic commented May 24, 2019

@edent it looks like your affiliation with the alphagov GitHub organization is not public; could you make it so per https://help.github.com/en/articles/publicizing-or-hiding-organization-membership to allow the IPR checker to sign off?

@edent
Copy link
Contributor Author

edent commented May 24, 2019

@domenic I am no longer a member of @alphagov, although I was when I made the PR.

I am on secondment to @nhsx, which is part of the UK Government. If that helps?

@annevk
Copy link
Member

annevk commented May 27, 2019

@edent we'd need NHSX to sign the agreement, I think.

Although maybe since the contribution was made while you were at alphagov and were their contact person we can override the bot in this case...

@edent
Copy link
Contributor Author

edent commented May 27, 2019

Won't be able to get NHSX to sign any time soon, so if you could override the bot that'd be great.

@annevk annevk closed this Jan 15, 2021
@annevk annevk deleted the branch whatwg:master January 15, 2021 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants