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

Remove table sorting model #526

Closed
wants to merge 1 commit into from

Conversation

Ritsyy
Copy link
Contributor

@Ritsyy Ritsyy commented Jan 17, 2016

Fix #345

@Ritsyy Ritsyy force-pushed the Removetablesortingmodel branch 2 times, most recently from 6be3442 to e40d7bc Compare January 17, 2016 16:03
@annevk
Copy link
Member

annevk commented Jan 18, 2016

@Ritsyy when I run the build script against this patch I get ten errors. I also noted that you remove some examples that include images. We should probably remove the images at the same time.

@Ritsyy Ritsyy force-pushed the Removetablesortingmodel branch from e40d7bc to 4f4767d Compare January 18, 2016 15:59
@Ritsyy
Copy link
Contributor Author

Ritsyy commented Jan 19, 2016

@annevk In this too

@annevk
Copy link
Member

annevk commented Jan 19, 2016

@Ritsyy I still get ten errors when I run the build script.

@domenic domenic added the removal/deprecation Removing or deprecating a feature label Jan 20, 2016
@Ritsyy Ritsyy force-pushed the Removetablesortingmodel branch 5 times, most recently from 07ef488 to 9582600 Compare January 25, 2016 18:55
@Ritsyy
Copy link
Contributor Author

Ritsyy commented Jan 26, 2016

@annevk I have committed the changes.

@annevk
Copy link
Member

annevk commented Jan 27, 2016

The wrapping here is wrong:

  <p>The <dfn><code data-x="dom-th-abbr">abbr</code></dfn> IDL attribute must <span>reflect</span>
  the 
  content attributes of the same name.</p>

That can be two lines.

The wrapping here is wrong too:

   data-x="attr-select-multiple">multiple</code> attribute or a <span data-x="concept-select-size">
   display size</span> greater than 1, and elements that would not be <span>interactive content
   </span> except for having the <code data-x="attr-tabindex">tabindex</code> attribute

You cannot have a newline/whitespace following a start tag or preceding an end tag (both happen here).

Looks good otherwise.

@foolip
Copy link
Member

foolip commented Jan 28, 2016

Maybe we should have a tool to auto-format the spec so we don't have to ask people to learn the style :)

@Ritsyy Ritsyy force-pushed the Removetablesortingmodel branch from 9582600 to a2368ca Compare January 28, 2016 08:26
@foolip
Copy link
Member

foolip commented Jan 28, 2016

@Ritsyy, I see you have fixed @annevk's wrapping issues.

I have also reviewed the changes, very nice! Here's how I will change the commit message when merging:

Fix #345: Remove table sorting model

Related commits:
https://github.com/whatwg/html/commit/6db0d8d4e3456140de958c963afe9bb9ec7b6a25
https://github.com/whatwg/html/commit/f1b702f93a52c69993aed5b690104d41ffef53ee
https://github.com/whatwg/html/commit/427c247ba3ffd61db556b4edfd2992e3ac7e6241
https://github.com/whatwg/html/commit/2699e4a3951a2c3e7b7e740d475b43f67d0b9f63
https://github.com/whatwg/html/commit/776ac74574c176c253713646eeb6216bee80dea5

Note the blank line after the first line, and of course the link to the issue. Thanks again!

foolip pushed a commit that referenced this pull request Jan 28, 2016
@Ritsyy
Copy link
Contributor Author

Ritsyy commented Jan 28, 2016

@foolip Thank you for the reviews!, will remember this now :)

@foolip
Copy link
Member

foolip commented Jan 28, 2016

Pushed as commit 59b7e24

@cvrebert
Copy link
Member

cvrebert commented Aug 7, 2016

X-Ref: w3c/html#248

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
removal/deprecation Removing or deprecating a feature
Development

Successfully merging this pull request may close these issues.

5 participants