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 to Bikeshed #49

Closed
wants to merge 5 commits into from
Closed

Convert to Bikeshed #49

wants to merge 5 commits into from

Conversation

foolip
Copy link
Member

@foolip foolip commented Sep 13, 2016

Optimized for review, when the sum of changes looks good I'll turn it into a single commit.

@foolip
Copy link
Member Author

foolip commented Sep 13, 2016

Oh right, this doesn't rewrap anything, I think it makes sense to do that at the same time, WDYT?

<script id=head src=https://resources.whatwg.org/dfn.js defer></script>

<pre class=anchors>
urlPrefix: https://dom.spec.whatwg.org/
Copy link
Member

Choose a reason for hiding this comment

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

Since DOM is in bikeshed, we should not need this I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks some things can be used without this block, like "node document". I'll trim it down to just the ones that don't work without this.

@annevk
Copy link
Member

annevk commented Sep 13, 2016

Rewrap would be nice.

Also, an initial commit that does the renames of Overview.src.html to fullscreen.bs and Overview.html to fullscreen.html would be good (and does no other changes). That way there's less to remove and more git blame.

@foolip
Copy link
Member Author

foolip commented Sep 13, 2016

I've rewritten the history to do the rename first, net changes are the same.

@foolip
Copy link
Member Author

foolip commented Sep 13, 2016

Ready for proper review now.

@@ -534,18 +582,18 @@ user.
<code>top</code> is zero.
</ul>

<p>To <dfn>add</dfn> an <var>element</var> to a
<p>To <dfn id=concept-top-layer-add>add</dfn> an <var>element</var> to a
Copy link
Member

Choose a reason for hiding this comment

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

These should probably have for="top layer" too?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's not enough to remove the need for the id, and also isn't needed to disambiguate. Is it so that HTML could link to this if it used Bikeshed? No other specs use top layer.

@annevk
Copy link
Member

annevk commented Sep 14, 2016

We should probably also add export to various <dfn> elements?

@foolip
Copy link
Member Author

foolip commented Sep 14, 2016

I'll export the terms used by HTML, are there any others you think are needed?

@foolip
Copy link
Member Author

foolip commented Sep 14, 2016

Well, HTML says "When an element is added to the pending dialog stack, it must also be added to the top layer. When an element is removed from the pending dialog stack, it must be removed from the top layer." without using Fullscreen's (new) add/remove definitions.

How about exporting only add/remove and treating top layer itself as an internal concept?

@foolip
Copy link
Member Author

foolip commented Sep 14, 2016

Strike that, the concept itself needs to be exposed in order to say "document's top layer".

@foolip
Copy link
Member Author

foolip commented Sep 14, 2016

Comments addressed, added for="top layer" too just in case.

This was used to compare IDs and links to the current spec:

for (var e of document.querySelectorAll('*')) {
  if (e.id)
    console.log('id: ' + e.id);
  if (e.hasAttribute('href') && e.className != 'self-link')
    console.log('href: ' + e.getAttribute('href'));
}

Existing IDs/links affected:
 * #table-of-contents → #toc
 * #refsCSS → #biblio-css (and similar)
 * #anolis-references is gone

The old and new spec were also copied into plaintext and compared to
verify no accidental differences. The biggest differences are in the
references section.
@foolip
Copy link
Member Author

foolip commented Sep 14, 2016

Now ready to commit, as a single commit with this message:

Editorial: convert to bikeshed

Steps to reproduce:

 * Convert fullscreen.bs by pasting into this one-off tool:
   https://gist.github.com/foolip/f60a1614fb5d194aad4dbff89c54ceb3
 * Manually finish conversion to Bikeshed, tweaking as needed to match
   the existing output as closely as possible.
 * Export top layer and its add/remove concepts for HTML.
 * Rewrap to 100 columns, taking care not to wrap inside elements.
 * Update README.md and remove Makefile.

(See #49 for branch with separate commits.)

Finally, compare the new fullscreen.html with the old:

  for (var e of document.querySelectorAll('*')) {
    if (e.id)
      console.log('id: ' + e.id);
    if (e.hasAttribute('href') && e.className != 'self-link')
      console.log('href: ' + e.getAttribute('href'));
  }

Existing IDs/links affected:

 * #table-of-contents → #toc
 * #refsCSS → #biblio-css (and similar)
 * #anolis-references is gone

The old and new fullscreen.html were also copied from a browser into
plaintext and compared to verify no accidental differences. The biggest
differences are in the references section.

Fixes #47.

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.

None yet

2 participants