Skip to content

Add aria-labels and accessibility improvements to buttons and navigation... #894

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

eslteacher902010
Copy link

@eslteacher902010 eslteacher902010 commented Jul 6, 2025

I added some aria labels. Some of the aria labels also add language filtering components. I hope this works.

I'm new to this, so appreciate your patience. Thanks!

Addresses #874

@coseeian
Copy link
Collaborator

@ksen0 I have reviewed this PR, and it looks good.

Copy link
Member

@ksen0 ksen0 left a comment

Choose a reason for hiding this comment

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

Hi @eslteacher902010 thank you for working on this! Happy to merge - just a very small request about the indentation.

<Icon kind={isOpen ? "chevron-down" : "chevron-up"} />
</div>
</button>
class={styles.toggle}
Copy link
Member

Choose a reason for hiding this comment

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

Could you fix the indentation here, please? Looks like some spaces got dropped

Copy link
Author

Choose a reason for hiding this comment

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

Same here thanks!

aria-hidden="true"
tabIndex={-1}
>
class={styles.toggle}
Copy link
Member

Choose a reason for hiding this comment

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

Same spacing issue here- thank you!

Copy link
Author

Choose a reason for hiding this comment

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

I tried to fix it, but let me know if I'm off in any way. Thanks!

@ksen0
Copy link
Member

ksen0 commented Jul 18, 2025

Hi @eslteacher902010 Thanks for your work on this! Unfortunately, it looks like both this and your other PR got mixed together. This PR now contains the unrelated commit to improve the contrast function. I need to review the new PR, but in the meantime, do you mind creating a new PR with only the commits that apply to the aria-labels issue? (If there's another straightforward way todo it without making a new PR please go ahead, but the instructions below are the most direct way I can think of.)

In your local fork:

git checkout main
git pull origin main
git checkout -b aria-labels-pr
git cherry-pick 0b6fce9166dbbe4164731e3efec476f8ce66b9f3 c81694d54980667e4ce330310f357c9366dfdb27 314036b37662acb183e99e77935d02f309c65469
git push origin aria-labels-pr

In the description, you can mention this PR and the issue, and I can close them as needed once merged. The cherry-pick command takes only the relevant commits (the first 3 in their PR) and leaves out the one that was accidentally added. In the future, I'd recommend creating separate branches (with git checkout -b your-branch-name) to avoid this if you're working on multiple PRs at once.

Please let me know if you need any support with any of that, Im happy to help!

@eslteacher902010
Copy link
Author

Hi @ksen0 , I created a new pull request as you suggested. While following your instructions, I ran into some conflicts in MainNavLinks.tsx and JumpToLinks.tsx — mostly related to overlapping changes and indentation. I did my best to resolve them and match the formatting, so I hope it's all is good now but let me know!

Also, just out of curiosity — how can I avoid running into this kind of issue again in the future? I really appreciate your help and guidance.

@ksen0
Copy link
Member

ksen0 commented Jul 18, 2025

Hi @eslteacher902010 , it's no worries at all! I'll keep my comments in this thread for clarity.

Looks like I didn't quite get it right with the commands in my code snippet above, because when I look at the new PR the contrast commit is still in there, and the other commits are duplicated.

Here is corrected code - could you try this again please? Sorry!

git remote add upstream https://github.com/processing/p5.js-website.git 
git fetch upstream
git checkout -b aria-labels-pr-2 upstream/main
git cherry-pick 0b6fce9166dbbe4164731e3efec476f8ce66b9f3 c81694d54980667e4ce330310f357c9366dfdb27 314036b37662acb183e99e77935d02f309c65469
git push origin aria-labels-pr-2

The mistake I made in the code of my previous comment is that "origin" (your fork, or github.com/eslteacher902010/p5.js) is now already different than "upstream" (github.com/processing/p5.js). So "git checkout main / git pull origin main" will refresh from the main branch of your fork, not the upstream - and if your fork already has the contrast commit, then that's not the goal. Rather, the above code first gets the more recent main from the processing/p5.js "upstream" repository, and then makes a branch in your fork based on that: notice that in git checkout -b aria-labels-pr-2 upstream/main there is the "upstream/main")

Can you try 1 more time please / with a fresh PR? You mentioned you had some conflicts, but if it's successfully starting clean, then there should be none of the indentation issues. Sorry for the mistake on my end, I hope it works but I'm happy to keep sorting it out! The cherry-pick process is pretty tricky, I tried to to explain but I'm happy to clarify anything as well as I can.

To avoid this in the future, either of these 2 options works:

  1. Work in "main" branch of your fork, only on 1 PR at a time (might be limiting because it sometimes takes a while to finish a PR)
  2. Always create a new branch for each new issue (normally this would be git checkout -b branch-name-here but you can do the method above with upstream/main to make sure it's cleanly based on the latest version of processing/p5.js). I like to give them 1 or 2 word names that are descriptive, so I don't get lost

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants