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 punc, spelling, inconsistent labels, etc. #667

Merged
merged 3 commits into from
May 25, 2023
Merged

fix punc, spelling, inconsistent labels, etc. #667

merged 3 commits into from
May 25, 2023

Conversation

TallTed
Copy link
Member

@TallTed TallTed commented Oct 21, 2022

fixes #661

basic-rec-track.svg Outdated Show resolved Hide resolved
basic-rec-track.svg Outdated Show resolved Hide resolved
Copy link
Collaborator

@frivoal frivoal left a comment

Choose a reason for hiding this comment

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

There are some overlapping/redundant labels. They were not created by this PR, but after this PR, they don't align exactly, and that makes things look weird and unreadable.

@TallTed
Copy link
Member Author

TallTed commented Oct 24, 2022

@frivoal — I have no problem changing the partial overlap to be complete, with the new text taking precedence on the latter instance. I would also have no problem deleting one or the other if that makes sense.

Oddly, GitHub is not offering me a "commit suggestion" button, so I'll have to manually apply the changes. Doing so now to make the obscuring overlap clear. Will delete whichever instance (if either) is determined to be unnecessary whenever that happens.

@TallTed
Copy link
Member Author

TallTed commented Oct 24, 2022

Digging deeper... @fantasai @frivoal

The rendered SVG (now) shows no CR to CRD, so it seems the comments in the source may need some updating. I think —

  • all instances of Candidate Recommendation without suffix should become Candidate Recommendation Snapshot
  • <!-- Advance to CR --> (line 43) should become <!-- Advance to CRS -->
  • <!-- CR --> (line 56) should become <!-- CRS -->
  • <!-- CR to CRD --> (line 71) should become <!-- CRS to CRD -->
  • <!-- CRD to CR --> (line 97) should become <!-- CRD to CRS -->
  • <!-- Update CR in place --> (line 111) should become <!-- Update CRS in place -->
  • <!-- Return to CR --> (line 176) should become <!-- Return to CRS -->

Related to this, <!-- Update CR in place --> (line 111, to become <!-- Update CRS in place -->) does not appear to have any labels in the rendered SVG, though there are two in the source (lines 118–119) which are the ones that overlay labels on other sections (lines 105 & 119, labeling <!-- CRD to CRS -->; and 78 & 118, labeling <!-- CRS to CRD -->).

If you agree with the above, please +1 or similar, and I'll put them all into my PR. If you've partial disagreement, please note that as well, and I'll update what's agreed, and we can continue to discuss what's not agreed.

@fantasai
Copy link
Collaborator

This diagram has been reviewed for understandability by screenreaders, so whatever you do, don't break that.

@frivoal
Copy link
Collaborator

frivoal commented Oct 25, 2022

This diagram has been reviewed for understandability by screenreaders, so whatever you do, don't break that.

Maybe the easiest thing to do then is to move back the label by 2px so that it lines up again.

@TallTed
Copy link
Member Author

TallTed commented Oct 25, 2022

This diagram has been reviewed for understandability by screenreaders, so whatever you do, don't break that.

Maybe the easiest thing to do then is to move back the label by 2px so that it lines up again.

Yes, I think the 2px nudge is the right answer for the overlaying labels, because these labels actually do apply to two pathways, so for screenreaders, they should be present twice.

I am also changing all the  CR  in comments to  CRS , so the comments match the rendered image content (and the text being illustrated here).

@TallTed TallTed requested a review from frivoal October 25, 2022 17:25
@TallTed
Copy link
Member Author

TallTed commented Oct 25, 2022

@fantasai — You're not in the GitHub reviewer list, so I can't ask for that as such ... but here's a free-text request for your review. :-)

@frivoal frivoal requested a review from fantasai October 26, 2022 03:17
Copy link
Collaborator

@fantasai fantasai left a comment

Choose a reason for hiding this comment

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

Mmmm, I'm not sure you wanted my review. :] Here's my full review:

  • If you're intending to obsess over punctuation, I'm going to require you to use curly quotes properly. :)
  • I would remove the “such as” clause for republishing WD, it's wordy and doesn't seem helpful.
  • I would change “returned for further review” to “returned for further work” on X -> WD
  • It's not clear whether the “such as” clause for PR->CRS is attached to both or to Team Decision only, and I'm not sure it's helping, so I would remove it also.
  • Team Decision after AC Review should be “W3C Decision”.
  • CRS->PR needs a WG Decision in addition to Team Approval.
  • Need to be consistent in labeling the WG Decision + Team Approval bits. Some of them have an “and”, some of them don't. I suggest using a plus sign throughout, like in the old version. (This is a diagram, not free text.)

(I also think your extended “such as” clauses are too wordy for a diagram, but I can live with it.)

@TallTed
Copy link
Member Author

TallTed commented Nov 21, 2022

@fantasai — Keeping in mind that I did not create the diagram, only cleaned up a number of existing inconsistencies and (to my eye) errors, I think several of your bullets are not really appropriate to a review of my cleanup PR.

Note: You can see exactly what I changed (vs. what I was changing and/or left alone) by viewing the files changed by the PR and clicking the <> button ("Display the source diff") at the upper right of the display pane. At the moment, this is a direct link — but that link will change as the PR changes. It is probably better to make future reviews and/or suggestions by looking at this page, where you can be exact about where your suggestion applies — and know whether you're addressing my PR or the previously existing content.

In more detail —

  • If you're intending to obsess over punctuation, I'm going to require you to use curly quotes properly. :)

My tools don't insert curly quotes unless I force them to. I generally avoid "smart" curly quotes because many automated tools choke on them in varying ways. They also tend to be used inconsistently and varyingly by international authors and editors, while straight quotes are usually consistently used.

If curly quotes are to be used, I strongly advise using the HTML or other relevant escapes for those characters, because the characters themselves are often misread and mistreated by subsequent editors — variously leading to pairs of left-quotes, pairs of right-quotes, reversed left- and right-quotes, mixes of curly and straight quotes, and other offenses against correct punctuation.

Also note that the W3C Manual of Style says to Use quotation marks rather than grave accents and apostrophes to quote text (e.g., ``value'' should read "value"). Note that this example itself uses straight quotes, suggesting (though not stating) that straight quotes are W3C style.

  • I would remove the “such as” clause for republishing WD, it's wordy and doesn't seem helpful.

Unless the thing(s) following any "such as" instance are a full list of things that might be put there, something like such as (which might include e.g. if punctuated correctly) is appropriate. I know this is "just a diagram" but I also know that people reading documents like this often treat the diagrams as equivalent to the nearby text (even when the document text — and/or the diagram itself — explicitly says they are not equivalent).

  • I would change “returned for further review” to “returned for further work” on X -> WD

Please note that "further review" was the existing text. I am happy to make this change, but perhaps it should be in a different PR?

  • It's not clear whether the “such as” clause for PR->CRS is attached to both or to Team Decision only, and I'm not sure it's helping, so I would remove it also.

I changed an "e.g." into "such as", so again, I'm not sure dropping that clause should be done via this PR.

Upon re-reading the whole diagram, I wonder whether the "AC Review and/or W3C Decision" on this vector (which had used , previously) should be and rather than and/or?

  • Team Decision after AC Review should be “W3C Decision”.

To be clear, this is on the PR->REC vector? (I wonder if that should change to "AC Review and W3C Decision", as it seems that both are required?)

  • CRS->PR needs a WG Decision in addition to Team Approval.

Again, not my text. Happy to apply this, if confirmed appropriate for this PR.

  • Need to be consistent in labeling the WG Decision + Team Approval bits. Some of them have an “and”, some of them don't. I suggest using a plus sign throughout, like in the old version. (This is a diagram, not free text.)

This is indeed a diagram. Still, it will pass through text readers and other accessibility tools, and they will treat + differently than and or & or other alternatives. Note that I changed all + to and, but I did not delete any + or and entirely, so any that are missing now were missing before. Again, I'm happy to add them, upon confirmation that they should be added through this PR.

@fantasai
Copy link
Collaborator

fantasai commented May 24, 2023

@TallTed, I pushed a set of changes to this branch that I think address all the relevant concerns with this diagram afaict. I think this now correctly represents the decision pathways.

I did remove the “such as” clauses, because they're a bit much for the diagram given how many other things are going on here. (And also they weren't particularly accurate anyway.)

Let me know what you think.

@fantasai fantasai added the Agenda+ Marks issues that are ready for discussion on the call label May 24, 2023
@css-meeting-bot
Copy link
Member

The Revising W3C Process CG just discussed Improvements to REC track diagram, and agreed to the following:

  • RESOLVED: Merge #667 to improve REC track diagram
The full IRC log of that discussion <fantasai> Subtopic: Improvements to REC track diagram
<fantasai> github: https://github.com//pull/667
<fantasai> florian: Back and forth between TallTed and fantasai
<fantasai> ... to improve the diagram and align it with current text
<fantasai> ... afaict it is now pretty correct
<cwilso> +1
<fantasai> plh: Any objections to approve?
<fantasai> RESOLVED: Merge #667 to improve REC track diagram

@css-meeting-bot css-meeting-bot removed the Agenda+ Marks issues that are ready for discussion on the call label May 24, 2023
@fantasai fantasai merged commit c1c53bc into w3c:main May 25, 2023
2 checks passed
@frivoal frivoal added this to the Process 2023 milestone May 25, 2023
@frivoal frivoal added Type: Editorial improvements Closed: Accepted The issue has been addressed, though not necessarily based on the initial suggestion labels May 25, 2023
@TallTed TallTed deleted the patch-1 branch May 31, 2023 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed: Accepted The issue has been addressed, though not necessarily based on the initial suggestion Type: Editorial improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

editorial: textual inconsistencies in basic-rec-track.svg
5 participants