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

Merge Accessibility Add-On into p5.js #4703

Merged
merged 74 commits into from
Aug 26, 2020
Merged

Conversation

lm-n
Copy link
Member

@lm-n lm-n commented Jul 21, 2020

This PR merges the text output and table output functionalities of p5.accessibility into p5.js by creating two functions:

-textOutput()
textOutput() creates a screenreader accessible output that describes the shapes present on the canvas. The general description of the canvas includes canvas size, canvas color, and number of basic shapes in the canvas example: 'Your output is a, 400 by 400 pixels, lavender blue canvas containing the following 4 shapes:'). This description is followed by a list of shapes where the color, position, and area of each shape are described (example: "orange ellipse at top left covering 1% of the canvas"). Each element can be selected to get more details. A table of elements is also provided. In this table, shape, color, location, coordinates and area are described (example: "orange ellipse location=top left area=2").

-gridOutput()
gridOutput(), formerly called table output, lays out the content of the canvas in the form of a grid (html table) based on the spatial location of each shape. A brief description of the canvas is available before the table output. This description includes: color of the background, size of the canvas, number of objects, and object types (example: "lavender blue canvas is 200 by 200 and contains 4 objects - 3 ellipses 1 rectangle"). The grid describes the content spatially, each element is placed on a cell of the table depending on its position. Within each cell an element the color and type of shape of that element are available (example: "orange ellipse"). These descriptions can be selected individually to get more details. A list of elements where shape, color, location, and area are described (example: "orange ellipse location=top left area=1%") is also available.

PR Checklist

  • npm run lint passes
  • [Inline documentation] is included / updated
  • [Unit tests] are included / updated

[paging @lmccart 4 the 👀 ]

@lm-n lm-n requested a review from kjhollen July 21, 2020 22:46
@lm-n lm-n marked this pull request as draft July 21, 2020 22:46
@lm-n lm-n changed the title Experiment1 Merge Accessibility Add-On into p5.js Jul 27, 2020
@lm-n lm-n self-assigned this Jul 27, 2020
@lm-n
Copy link
Member Author

lm-n commented Aug 19, 2020

Looking good! Two main thoughts so far:

  • I think we will want a document that gives an overview of how the accessibility stuff works in general, placed in the contributor_docs folder. The former readme can serve as a starting point. What's missing from this doc currently when I think about what's in this PR, is a little more overview of how the different files fit together, and the flow that happens internally. This will be useful for making this work sustainable for future contributors to continue.
  • I left some notes throughout, but avoiding abbreviations in variable and function naming will also help ensure this code is readable.

Thanks Lauren! I've added a doc documenting how different files and functions fit together under contributor docs.

Copy link
Member

@kjhollen kjhollen left a comment

Choose a reason for hiding this comment

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

Hey Luis, I've still got some more to review in detail but wanted to share what I've done so far before we chat!

contributor_docs/web_accessibility.md Outdated Show resolved Hide resolved
contributor_docs/web_accessibility.md Outdated Show resolved Hide resolved
contributor_docs/web_accessibility.md Outdated Show resolved Hide resolved
contributor_docs/web_accessibility.md Outdated Show resolved Hide resolved
src/accessibility/color_namer.js Outdated Show resolved Hide resolved
src/accessibility/describe.js Outdated Show resolved Hide resolved
src/accessibility/describe.js Outdated Show resolved Hide resolved
@lm-n lm-n requested a review from kjhollen August 21, 2020 23:38
Copy link
Member

@kjhollen kjhollen left a comment

Choose a reason for hiding this comment

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

Hey Luis, this is in great shape. I had some questions about the return values (arrays of two objects) for some of the helper functions. I can see how this is going to cut down on calls traversing the DOM, and hope this results in some good performance improvements!

src/accessibility/color_namer.js Show resolved Hide resolved
src/accessibility/describe.js Outdated Show resolved Hide resolved
src/accessibility/describe.js Outdated Show resolved Hide resolved
src/accessibility/describe.js Outdated Show resolved Hide resolved
src/accessibility/describe.js Show resolved Hide resolved
src/accessibility/describe.js Outdated Show resolved Hide resolved
src/accessibility/describe.js Outdated Show resolved Hide resolved
src/accessibility/describe.js Outdated Show resolved Hide resolved
src/accessibility/describe.js Outdated Show resolved Hide resolved
src/accessibility/describe.js Outdated Show resolved Hide resolved
@lm-n lm-n requested a review from kjhollen August 25, 2020 16:06
@lm-n
Copy link
Member Author

lm-n commented Aug 25, 2020

Hi @kjhollen! I've added my project wrap up! Lmk what you think!

Copy link
Member

@kjhollen kjhollen left a comment

Choose a reason for hiding this comment

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

Hey Luis, this looks like it's ready to go. I found one little typo in the documentation. The comments that you've added are super helpful, and will make it easier for other folks to contribute new features and maintain the existing functionality. Thanks for all your work on this and your many rounds of revisions!

contributor_docs/web_accessibility.md Outdated Show resolved Hide resolved
src/accessibility/describe.js Outdated Show resolved Hide resolved
src/accessibility/describe.js Outdated Show resolved Hide resolved
@lmccart
Copy link
Member

lmccart commented Aug 26, 2020

@lm-n This is really fantastic work, I'm so excited about it. A big step forward for the p5.accessibility project. I'm looking forward to continuing this work with you all, in any way we can. Thank you for your time and energy this summer, and for the thorough writeup and documentation. And thanks to @kjhollen for mentoring this summer!

@lm-n
Copy link
Member Author

lm-n commented Aug 26, 2020

Hey Luis, this looks like it's ready to go. I found one little typo in the documentation. The comments that you've added are super helpful, and will make it easier for other folks to contribute new features and maintain the existing functionality. Thanks for all your work on this and your many rounds of revisions!

Hi @kjhollen ❤️ I've fixed the typo and moved the comments below the else! I think we should be ready to merge 😄 Thanks so much for all your help and support! @lmccart and @CleezyITP thank you ❤️, I'm excited to see what comes next!

Copy link
Member

@kjhollen kjhollen left a comment

Choose a reason for hiding this comment

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

did a local test after merging with the main branch this morning and all tests pass. :)

@kjhollen kjhollen merged commit 876875d into processing:main Aug 26, 2020
@LucaDamasco
Copy link

Just wanted to pop in and say this rocks. Really excited to be able to more easily share this work with my students and seeing that this was directly integrated into P5 after searching up p5.accessibility today was such an awesome sight! ❤️

@lm-n lm-n deleted the experiment1 branch March 28, 2021 19:40
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.

4 participants