-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Edit docs for accessibility #6484
Conversation
In |
@MsQCompSci thanks for pointing out the missing Should we generally use I updated the descriptions of |
I think the examples that don't use setup/draw are useful for showing very concise examples without boilerplate potentially distracting from the concept, e.g. to show a number of examples with the different parameters for the same function without the page getting super long. In general though, I think it's helpful having at least one example with the full boilerplate so that readers know what part of their sketch to add the function to. Maybe it would be most consistent to always do drawing in a |
@davepagurek @Qianqianye if these changes look good to you, please go ahead and merge them. |
* background(220); | ||
* background(200); | ||
* | ||
* let x = frameCount % 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be helpful to add a comment here to explain what frameCount % 100
does in the code
src/accessibility/describe.js
Outdated
* describe('green circle at x pos ' + round(x) + ' moving to the right'); | ||
* circle(x, 50, 40); | ||
* | ||
* describe('A green circle moves from left to right on a gray square. It restarts on the left edge after reaching the right edge.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original description shows that users can use variables like 'x' within describe()
. I think it's helpful to show this use case. What do you think? @nickmcintyre
* <div> | ||
* <code> | ||
* let x = 0; | ||
* function draw() { | ||
* textOutput(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to show textOutput(LABEL) in one of the example snippets?
* <div> | ||
* <code> | ||
* let x = 0; | ||
* function draw() { | ||
* gridOutput(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to show the gridOutput(LABEL) usecase in one of the example snippets?
* fill(0, 0, 255); | ||
* square(50, 50, 50); | ||
* | ||
* describe('A red circle and a blue square on a gray background.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to explain the use case of using both textOutput() and describe()?
Great suggestions @Qianqianye. Please let me know how the latest revisions look. |
Thanks @nickmcintyre, great work! I just merged it. Just to take a note for our backlog, some of labels in the example code are quite long, which takes up a lot of vertical space. We can revisit this after the web redesign phase. |
Addresses #6483
Changes:
I edited the inline docs for
describe()
,describeElement()
,textOutput()
, andgridOutput()
to bring them more in line with the style guide.@dkessner, @MsQCompSci, @davepagurek, @limzykenneth, @Qianqianye, @katlich112358, @calebfoss, @cosmicbhejafry, @apoorva-a98, @tedkmburu, @Zarkv, @SkylerW99, @itsjoopark, @hannahvy, @nhasalajoshi I'd love to incorporate any feedback you may have! Here's a staging version of the edits in this pull request.
PR Checklist
npm run lint
passes