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

Fix bikeshed indentation errors in web-animations-2 diff spec. #6565

Merged
merged 4 commits into from Sep 7, 2021

Conversation

flackr
Copy link
Contributor

@flackr flackr commented Sep 1, 2021

[web-animations-2] Fix bikeshed indentation errors.

The spec had several incorrect indents that lead to failures running bikeshed on it.

web-animations-2/Overview.bs Outdated Show resolved Hide resolved
@@ -216,18 +216,19 @@ follows:
1. Set |previous progress| based in the first condition that applies:
<div class="switch">

: If |previous current time| is unresolved:
: If |previous current time| is unresolved:
Copy link
Member

Choose a reason for hiding this comment

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

Indent this list one level so it's properly nested under its div; then you won't need the blank line to make it work properly. You can also swap out the div for a dl, so it'll collapse into the Markdown dl in the output rather than remaining as a wrapper.

(Can't do a suggestion for this, since it woudl involve editting unchanged lines.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For swapping to a dl, I'm not sure how to preserve the switch formatting. In the generated css, div.switch > dl > dt gives the custom markers but if i switch to just a dl then of course it doesn't have the wrapping div so the custom markers aren't used.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, the selector should just be keying off of the switch class, not the div tagname. I'll note, tho, that in the current draft it's not being displayed at all; it's possible that the custom marker is only in Bikeshed's stylesheet and not the W3C stylesheet. ^_^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's curious, on https://drafts.csswg.org/web-animations-2/ the styles for switch are specified on the dl element rather than the wrapping div. I guess there's something wrong with my local bikeshed?

Copy link
Member

Choose a reason for hiding this comment

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

Ah indeed, it's on dl.switch. Well, my point still stands that you should swap to a wrapper dl rather than a wrapper div so this actually works the way you intend. ^_^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After fixing all of them it seems like my local bikeshed is generating css for dl.switch now 🤷‍♂️

@@ -1014,6 +1015,7 @@ Add:
> matching condition from below:
>
> <div class="switch">
>
Copy link
Member

Choose a reason for hiding this comment

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

Same here - indent the markdown list and you won't need these blank lines to hack around the markdown parser. And swap the div for a dl as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've swapped all of them now ^_^

@@ -1032,6 +1034,7 @@ Add:
> Presently start and end delays are zero until such time as percentage
Copy link
Member

Choose a reason for hiding this comment

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

Might as well swap this out for a markdown paragraph starting with Note:, as I included in an earlier suggestion, too. Then you don't have to worry about the p's contents also being improperly indented. ^_^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

flackr and others added 2 commits September 1, 2021 12:56
@tabatkins tabatkins merged commit 1f280de into w3c:main Sep 7, 2021
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