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

Text spacing understanding reword #1457

Closed
wants to merge 5 commits into from
Closed

Conversation

mbgower
Copy link
Contributor

@mbgower mbgower commented Sep 19, 2020

I was working on something else, merely referencing this, but got pulled up short by the second paragraph of the Intent -- then got drawn into some other minor fixes. I don't believe anything I'm proposing in the first commit is adventurous.
The second commit, which strips out the second paragraph is more so, but I think is supportable.

Mild editing for common usage and readability.
I was unclear exactly what was meant by the second paragraph of the Intent section; however, the suggested minor edit seems to make it more sensical.
The User Responsibility section read very oddly. I have attempted to clarify while retaining its spirit.
Removed second paragraph. I tried, but really could not make sense of exactly what the paragraph was trying to convey.
The author responsibility section immediately following touches on the idea of the SC being a baseline which authors are encouraged to exceed.
I have put this in as a separate commit, in case it is felt the paragraph has value, making it easier to keep out of the PR without losing other editorial fixes.
@patrickhlauke
Copy link
Member

patrickhlauke commented Sep 19, 2020

how is the removal of that paragraph justifiable, since in my mind that paragraph addressed the "to at least" part of all the metrics in the bullet points?

then again, this ties into the discussion we had ages ago about this #635 and cuts across the edit that was then made in #860

@mbgower
Copy link
Contributor Author

mbgower commented Sep 19, 2020

Created a separate issue for what is some kind of generator glitch on the figures #1458

@mbgower
Copy link
Contributor Author

mbgower commented Sep 19, 2020

@patrickhlauke, please see my individual comments on commits. I put the removal in a separate commit so it is easy to reject and keep other chagnes. I found this paragraph nonsensical when I read it, and tried to just do a minor change in the first commit, which I think makes it more readable. But when I reread it after that change, it remains unclear to me exactly what it is saying (if it is more than what is talked about in the very next section, which I also addressed in my commit and PR comments).

Maybe you could explain what the paragraph is trying to explain, and if it is adds more to what follows 2 paragraphs later

The values in the SC are a baseline. Authors are encouraged to surpass the values specified, not see them as a ceiling to build to.

I'm fine to rewrite if I understand what needs to be said. I don't have a lot of skin in this game, but I do feel pretty strongly that as it stands the paragraph is of little value.

@mbgower
Copy link
Contributor Author

mbgower commented Sep 19, 2020

@patrickhlauke Is this what it's meant to say?

The metrics set a target for a minimum increase in text spacing that must be met. Starting from the author's baseline implementation, any increase in spacing for any of these four style properties up to and including these targets should not result in the loss of content or functionality.

That seems to me to be self-evident from the wording in the SC, but this at least makes sense to me, which the existing wording on its own did not.

I suggest this would fit better as the third paragraph of the Intent, rather than the second.

Let me know, and I'll push it as another commit.

@patrickhlauke
Copy link
Member

Is this what it's meant to say?

I believe so, yes (if i'm reading/understanding the lengthy to-ing and fro-ing from #635 and the resultant #860 right)

Addressing Patrick's comment
-trivial style edit in first paragraph.
-brought up user responsibility to immediately follow author responsibility, and removed last line in author responsibility, which was about the user.
- added line in author responsibility saying they are not responsible for providing a mechanism to alter text spacing (something I thought was still not clear).
Made Applicability a peer heading of these.
@mbgower
Copy link
Contributor Author

mbgower commented Sep 20, 2020

Implemented a replacement second paragraph.
On review, shifted the user responsibility section to immediately follow the author responsibility section, and added in clarification that the author is not responsible for providing a mechanism to alter text spacing. I think this is good to go now.

Base automatically changed from master to main March 5, 2021 20:42
@fstrr
Copy link
Contributor

fstrr commented Jul 9, 2024

@mbgower You still want this PR?

@patrickhlauke
Copy link
Member

This is so conflicted at this point, that I'd recommend redoing this as a fresh PR

Copy link

netlify bot commented Oct 30, 2024

Deploy Preview for wcag2 failed.

Name Link
🔨 Latest commit be0b4af
🔍 Latest deploy log https://app.netlify.com/sites/wcag2/deploys/672292d943f0c00008de8f22

mbgower added a commit that referenced this pull request Oct 30, 2024
Incorporated changes from #1457 into a clean PR
@mbgower
Copy link
Contributor Author

mbgower commented Oct 30, 2024

Redid this cleanly against main as #4125

@mbgower mbgower closed this Oct 30, 2024
@mbgower mbgower deleted the TextSpacingUnderstandingReword branch October 30, 2024 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants