Better HTML #52
Better HTML #52
Conversation
- remove unnecessary HTML elements - not-table content now does not use tables - add semantic elements - modify to be valid HTML5
em elements
|
This is very difficult to review. So many changes, touching most if not every line! It would be somewhat easier to review if each bullet point were its own commit ... but still not easily done. |
|
The changes are not that big, it's just that diff can't show it. |
|
As far as I can tell the changes are very big. Perhaps part of the changes are changing whitespace. But there is also the movement of markup to separate lines, which doesn't seem to be mentioned in the summary of the change. In my opinion it would have been better to not have the formatting changes. |
|
Yes, most of these changes are extra spaces or deleted spaces. To properly understand and improve the document, I first had to clean up the indentation. My editor does this automatically. |
|
It's a bit of a stretch to say the changes are big when nothing has changed in terms of content. |
|
I found it easier to view the changes by using the "Hide whitespace" checkbox (then "Apply and reload"). (Finding it may be harder; click the "gear" button under the "Files changed" tab.) |
|
Including whitespace, GitHub indicates 3,864 lines were changed. Excluding whitespace, GitHub indicates 1,940 lines were changed. (I do note that GitHub doesn't treat linefeed addition/deletion as whitespace changes.) Even if only one character was changed on each of those lines, and even if those changes were only to markup (e.g., fixing an |
|
@TallTed These single-character changes most often involve changing a single letter, e.g. the graph G, to |
Yes. And they are hard to review, all at once, all of a piece. They would be easier to review if there were many commits contained within this PR, e.g., "change all Reviewing commit-by-commit would be FAR easier than reviewing all-at-once, which is the only current option. |
There was a problem hiding this comment.
The nature of source formatting changes is that it can change a lot of lines. But, looking at it with the "ignore whitespace" option, and in the HTML Diff, it's clear that the changes are limited to formatting.
Note that this will collide with other open pull requests. We should probably merge those first, rebase this PR on that, and then merge this.
|
Well, it could have been devided, but I treated everything that is not semantic as one PR. On the other hand, I don't fully understand what the difference is in checking time 5*20% and 100%. Yes, it's a big PR because the current specification document is/was in a terrible state. |
|
The build error may indicate some underling issue, but a ReSpec problem prevents it from being displayed; or, it may just be a ReSpec issue itself. |
spec/index.html
Outdated
| <p>If <em>E</em> is a ground RDF graph, then <em>I(E) = false</em> if <em>I(E')</em> = | ||
| false for some triple <em>E'</em> in <em>E</em>, otherwise <em>I(E) = true</em>. |
There was a problem hiding this comment.
GitHub fails to highlight my suggested changes. Please use your preferred diff tool.
| <p>If <em>E</em> is a ground RDF graph, then <em>I(E) = false</em> if <em>I(E')</em> = | |
| false for some triple <em>E'</em> in <em>E</em>, otherwise <em>I(E) = true</em>. | |
| <p>If <em>E</em> is a ground RDF graph and <em>I(E') = false</em> | |
| for some triple <em>E'</em> in <em>E</em>, then | |
| <em>I(E) = false</em>; otherwise, <em>I(E) = true</em>. |
|
@TallTed Thanks for pointing out a few |
I believe the ReSpec issue is in reporting the underlying error. It does not error in the same way on the main branch. But, it could still be entirely a ReSpec internal thing. I'm hoping they can address/fix this one quickly. |
domel
added
the
spec:editorial
|
Closing, finally, as will not be done |
|
@pfps why did you close this PR? |
|
Sorry, I thought this was the one for better display on mobile devices. |
|
The Semantic Conditions for Ground Graphs have been subtly changed so that they no longer are correct.. This happens in a large change block where it is unclear just what the substantive change is. This and any other substantive changes need to be removed from the PR. It is unacceptable as is. |
|
This PR shows the pitfalls of using a mechanical style. The list of terminology was very much easier to see in the source when each item was one line. With each item taking up multiple lines is is much harder to get a notion from the source of what terminology is being used. |
|
Why was this merged? |
|
@domel @gkellogg — I fear this must be reverted, and its changes re-attempted via new PRs, with more specific changes in each, starting with one PR for each of the 29 commits that were a part of this PR. The comments above did NOT approve this PR, because we (at least @pfps and I) simply could not usefully review the 2000 lines of HTML and other changes, in this agglomerate shape. |
|
We agreed on 2 weeks before the merge. All suggestions were corrected. 4 weeks have passed. No one has reported corrections, no one has clicked on blocking merging. |
|
I said multiple times that I could not properly review the changes contemplated by this PR. I am sorry that I didn't click the UI buttons that apparently overrule human language. I say again — I could not properly review the changes in this PR. I do not approve of its merging. I have no useful way to review its impact. Combing through the entire document is not a viable option! |
|
I agree entirely with TallTed. |
|
@TallTed You made many suggestions for changes while at the same time claiming that it is impossible to review the text. |
|
@domel — I did make many suggestions. You then made similar changes above and beyond my suggestions (along the same lines as my suggestions), because my suggestions revealed to you that you had not made all your intended changes. To my mind, this makes plain that the PR was not properly reviewable, even by its author — because if it were, I would have included the extra changes you made in the changes I suggested! |
|
Two comments: 1. I think I have implemented all your suggestions. Each time I ask for a review. 2. I wonder why you are writing this now and not 4 weeks ago. I think it would be good if you refreshed this thread because I have the impression that you forgot a few important details and sequence of events. |
And that request goes into an intangible queue. This document, never mind this PR, is not always at the top of my task list.
4 weeks ago, the comment thread above was still fresh and awaiting input/reply from others. Now (3 days ago), the PR was merged, effectively (but silently) declaring that the comments above had "expired" even though unresolved/unaddressed. I have a problem with that. |
This PR addresses several HTML structure and semantic issues to improve accessibility, maintainability, and adherence to best practices. The following corrections were implemented:
See #51
Preview | Diff