activist-org / activist

An open-source activism platform
https://activist.org
GNU Affero General Public License v3.0
229 stars 187 forks source link

Improve the alternate text of all buttons #151

Closed andrewtavis closed 1 year ago

andrewtavis commented 1 year ago

Terms

Description

This issue is to go through and add better alternate texts for all buttons on the website. These texts should be added to the English i18n file so that we can remember to localize them. As an example: the Learn more buttons on the home page all need alternate texts that are more like, "Learn more about getting involved in an activist organization."

Contribution

This is a great first issue for someone who'd like to get some experience with the project :) Happy to support someone who has interest!

Gauravvpnd commented 1 year ago

Hey, i want to work on this issue can you please explain me a little?

andrewtavis commented 1 year ago

Hey @Gauravvpnd! Thanks for your interest in activist :) This issue is specifically about accessibility for people who need screen readers. An issue with the website right now is that if someone is using a screen reader they would hear a button like Learn more, but then they wouldn't necessarily know what they'd be learning more about without investigating other parts of the page or just choosing to use the link and find out.

It'd be great if there would be alternate texts for people with screen readers that were more descriptive of the eventual destination, so as in my example we could have the alternate text for the screen reader be "Learn more about getting involved in an activist organization."

I think the easiest way of achieving this would be to add a prop to frontend/components/LabeledBtn.vue that would take in the alternate text and assign it to the button. From there we could then add some longer texts to the English i18n file.

Let me know if this sounds like something you'd be interested in working on! Would of course be happy to explain more 😊 If it does sounds like something you want to work on, then I'd be happy to assign you :)

Gauravvpnd commented 1 year ago

Thanks for explaining this sounds interesting I would love to work on this. you can assign me this

andrewtavis commented 1 year ago

Assigned! Let me know if you need some help :)

Gauravvpnd commented 1 year ago

I have created a pull request doing the changes. You can have and look at and merge it.

andrewtavis commented 1 year ago

Thanks @Gauravvpnd! Will check this in the coming days :) :)

andrewtavis commented 1 year ago

Unassigning for now due to minimal activity in the PR.

luuu-xu commented 1 year ago

This indeed looks like a great first issue for me at activist! Please assign me and I will look into more readings on best practices etc, play around and try solving it!

andrewtavis commented 1 year ago

Great, @luuu-xu! Let me know if you have any questions :) #156 also has some documentation that might be helpful in this 😊

luuu-xu commented 1 year ago

@andrewtavis After some hard learnings of Vue, I have implemented conditionally added aria-label to buttons on index page. Please give a review to the PR and let me know if I should keep working through all pages in this project.

andrewtavis commented 1 year ago

Thanks, @luuu-xu! I'll check it soon and give you some feedback 😊 Hope the Vue learning was a good time!

luuu-xu commented 1 year ago

@andrewtavis Please let me know if the PR needs any changes. Any tips and suggestions are greatly appreciated too! I would love to keep working on this project.

andrewtavis commented 1 year ago

Hey @luuu-xu! Appreciate you reaching out to check in on this :) Sorry that the review is taking a little longer than expected. I’m in the midst of moving to a new apartment right now and am thus a bit slower with all of this than usual.

We have a meeting tomorrow to discuss the backend, which you’d be welcome to attend if you have interest. The details are in the dev channel on Matrix :) I hope to get to this review after that meeting in the evening or on Sunday at the latest. We can then work to get these changes pushed to other buttons on the platform!

Thanks again for your determination! 😊

Yadnu commented 1 year ago

I want to work on this issue

andrewtavis commented 1 year ago

Hi @Yadnu! Thanks for reaching out! @luuu-xu has this one covered for now, but there are going to be a lot more issues being made after a developer sync we'll be having in a few hours :) Please check in then and we can figure out something for you to work on 😊

andrewtavis commented 1 year ago

@luuu-xu, just merged #204 😊 Thanks again so much, and sorry for the wait! As I said I'm moving atm 🚚 You'd be more than welcome to continue to add alternate texts to buttons for this issue in the same way that you did in #204. A quick note, for the landing page we still need to add labels for the Become a supporter and View all supporters buttons at the bottom :)

Thanks again! Really is great to have someone who's interested in supporting accessibility! Looking forward to working more with you πŸ™ƒ

luuu-xu commented 1 year ago

@andrewtavis Thank you for giving the PR a review and accepted the merge. The move must have been exhausting!

I will go ahead and add labels for index page's Become a supporter and View all supporters buttons and directly submit another PR for you to review.

Then I will scan through other pages and add labels for other ambiguous buttons. I'm assuming that I don't need to open another issue for them right? I will just create PRs accordingly.

By the way, while I was browsing our pages, I noticed that our tab support is not great. For example, the index page seems to have only one or two buttons that are tab selectable. I would like to open an issue and read into it and make PRs to fix them. Let me know what you think about this tab control issue.

andrewtavis commented 1 year ago

@luuu-xu, thank you for the contribution! Move's still going given we're still partially living out of boxes, but at least I have my desk set up 😊

I will go ahead and add labels for index page's Become a supporter and View all supporters buttons and directly submit another PR for you to review.

Sounds wonerful! πŸŽ‰

Then I will scan through other pages and add labels for other ambiguous buttons. I'm assuming that I don't need to open another issue for them right? I will just create PRs accordingly.

Also sounds good :) Feel free to combine ambiguous button labels into single PRs as you see fit :)

By the way, while I was browsing our pages, I noticed that our tab support is not great. For example, the index page seems to have only one or two buttons that are tab selectable. I would like to open an issue and read into it and make PRs to fix them. Let me know what you think about this tab control issue.

What's your browser? I'm very up to getting tab support stronger on the platform. For me it seems ok-ish as of now, and I can get to everything on the landing page fine with a tab – even the drop downs. With that being said, tab support on the home page is horrendous as of now...

For this, feel free to open an issue, and just let me know if you're also having problems with the landing page, or if you meant another page when you said "index page" πŸ™ƒ

Hope you had a great weekend!

luuu-xu commented 1 year ago

@andrewtavis

What's your browser? I'm very up to getting tab support stronger on the platform. For me it seems ok-ish as of now, and I can get to everything on the landing page fine with a tab – even the drop downs. With that being said, tab support on the home page is horrendous as of now...

So I went ahead and rechecked our landing page with different browsers:

  1. On Chrome, it is working great! On a glance I think every button/links are tab-selectable.
  2. For Firefox, only the dark mode and language selectors on navbar are tab-selectable.
  3. And on Safari, funny enough, nothing is tab-selectable!

If you think this will bis a good issue, I will go ahead and create the issue for our landing page first, and please assign me to it so I can read into best practices and stuff to make every browser supports tab-select as good as on Chrome!

Oh by the way, I noticed that for our buttons, those orange ones, we are only having 2.1 contrast ratio and it could be improved. One not so great solution would be changing the orange theme colour to a darker one but I don't think we should change our theme colour easily. The other solution will be changing the text-colour in our orange buttons from white to black/dark grey, this way we can have AAA ratings!

Right, I also find that according to mdn, Content with images must be labeled, some image contents like supporter organization links and our logo links, do not have labels for a11y users. This should be an easy fix and I think I can get them all solved with my coming landing page PR.

andrewtavis commented 1 year ago

And on Safari, funny enough, nothing is tab-selectable!

Welcome to development for Safari! πŸ€¦β€β™‚οΈπŸ˜…

If you think this will bis a good issue, I will go ahead and create the issue for our landing page first, and please assign me to it so I can read into best practices and stuff to make every browser supports tab-select as good as on Chrome!

By all means create the issue and I'll assign! πŸ™Œ

Oh by the way, I noticed that for our buttons, those orange ones, we are only having 2.1 contrast ratio and it could be improved. One not so great solution would be changing the orange theme colour to a darker one but I don't think we should change our theme colour easily. The other solution will be changing the text-colour in our orange buttons from white to black/dark grey, this way we can have AAA ratings!

Really appreciate your consideration for this, @luuu-xu! 😊 You can also make an issue for this and I'll check it in the designs first, and then after that we can make the switch in code. I'd also assign that to you if you'd have interest :) :)

Right, I also find that according to mdn, Content with images must be labeled, some image contents like supporter organization links and our logo links, do not have labels for a11y users. This should be an easy fix and I think I can get them all solved with my coming landing page PR.

Sounds perfect! Really looking forward to the PR! Let's be in touch after that on the other issues and potentially set up a call at some point πŸ™ƒ

luuu-xu commented 1 year ago

If you think this will bis a good issue, I will go ahead and create the issue for our landing page first, and please assign me to it so I can read into best practices and stuff to make every browser supports tab-select as good as on Chrome!

Apparently the reasons that Safari and Firefox's tab select do not work well are different and not because of our website's support.

For Safari, users can just easily toggle the setting and it works great! Browser keyboard navigation in macOS. I am very surprised that this setting is not on by default!

And for Firefox, there is a bug of Firefox on MacOS Catalina about tabfocus. The good news is that I found a fix here Firefox, let's focus, please! and now my Firefox works well too.

As I was working through tabfocus supports on our website, as I wrote in the PR #212, I came across the modal popups that do not feel great with tab and I went ahead and made some changes to them. Hopefully it is not causing a trouble for you to review them.

Really appreciate your consideration for this, @luuu-xu! 😊 You can also make an issue for this and I'll check it in the designs first, and then after that we can make the switch in code. I'd also assign that to you if you'd have interest :) :)

I will go ahead and write up a issue for our a11y contrast problems as we discussed, let's fix them!

andrewtavis commented 1 year ago

Really appreciate your support here, @luuu-xu! 😊 I'll try to get to the review in #212 soon and we can go from there. And don't worry about fixing the tab issues for models in there as well. You're welcome to get to things as you find them at this stage :) :)

andrewtavis commented 1 year ago

@luuu-xu, I think we can get back to this and finalize it after #300 os merged. What do you think? From there we can add something to the contribution guide to let people know that it’s expected to add these texts :)

andrewtavis commented 1 year ago

The localization changes just went through, @luuu-xu 😊 The line here shows how aria labels are defined given the new system. Let me know what you think on the new L10N key system, btw!

luuu-xu commented 1 year ago

@andrewtavis I love how the new structure for i18n is component based and page based! If we grow our contents in the future, I think we can even for sure refactor it into two files or even directory-based? Anyways, yes, we should add this i18n standard to our contributing.md and let everyone know about it.

andrewtavis commented 1 year ago

Hey @luuu-xu πŸ‘‹ Yes I really like the work that @felipolis did :) Great impetus, and I'm happy with the result of it all 😊 Not sure if we can go to multiple files in the future, but we can experiment with the options that Transifex has :)

I didn't add too much to the contributing guide for this as the documentation for it actually went into the style guide. Will do another minor update on the system now, and please let me know what you think of it!

andrewtavis commented 1 year ago

Hey @luuu-xu πŸ‘‹ ddab5db adds in a ton more localizable aria labels. I think we should be getting close to closing this πŸ₯³ Want to give this a look sometime soon? Also happy to schedule some time to look at it together if that would make sense 😊

luuu-xu commented 1 year ago

@andrewtavis Wow! Great works! Our i18n feature is going to be awesome. I took a look at the commit and they look good. We might have missed some but I don't think it's too big of a problem, the system is set up already so we can easily fix them later! I'll email you about scheduling a call with you, I have been quite busy with something else myself. πŸ₯Ή

andrewtavis commented 1 year ago

Send the email along when you can, @luuu-xu 😊 I'll leave this issue open for just a bit longer so I get pushed to do one more sweep through to ge the last of the non-keyed strings :)

andrewtavis commented 1 year ago

A final thing for this, @luuu-xu: we should standardize the way we're naming aria-label props. Maybe instead of alternate text we have just do ariaLabel? That way we know it's a prop if it's camelCase and that it's direct HTML if it's kebab-case? Or we could do aria-lbl for the prop on all components? As of now we have some that are alternateText and others that are other names.

Lemme know what you think and I'll finalize this :)

luuu-xu commented 1 year ago

@andrewtavis Yes! I was thinking about this issue too. I think ariaLabel and aria-label should work fine. So now if it's camelCase ariaLabel then it's a prop, while aria-label is regular HTML attribute.

andrewtavis commented 1 year ago

Thanks for the feedback, @luuu-xu! I’ll go through and fix this and do a final sweep for alternate texts while I’m at it :) :)

luuu-xu commented 1 year ago

@andrewtavis So you are working on changing the prop name? I was thinking about doing it myself. Most likely the changes are related to BtnLabeled. And while you are at it, you should also make the changes to BtnLabeled regarding the label prop:

<span v-if="label" class="mx-auto">{{ $t(label) }}</span>

should be changed to:

<span v-if="label" class="mx-auto">{{ label }}</span>

Because now we are passing the i18n text string as label prop to BtnLabeled whenever it's used, so within BtnLabeled we don't need to $t it. There are 3 lines that need to be corrected.

andrewtavis commented 1 year ago

Thanks @luuu-xu! I’ll commit those changes tomorrow and we can close this 😊

luuu-xu commented 1 year ago

@andrewtavis I want to grab your opinion while you are here. I am doing my final sweep of attributes reordering and ariaLabel changes. I found out that we have different usages of BtnLabeled's label and ariaLabel prop:

sometimes we are passing in i18n key (label="components.btn-labeled.get-in-touch") into label,

but sometimes we are passing in already translated string into label like this :ariaLabel="$t('components.btn-labeled.get-in-touch-aria-label')".

We need to decide which string to take in for BtnLabeled's label and ariaLabel props since we need to write the component with the decision.

Personally I prefer passing i18n key into BtnLabeled and use $t(label) inside BtnLabeled because this is the lowest level.

andrewtavis commented 1 year ago

I 100% agree, @luuu-xu! Let’s definitely just pass the keys. I’d noticed that as well and doubtless contributed to the multiple versions myself. Thank you for noticing and working on this! 😊

andrewtavis commented 1 year ago

Ok @luuu-xu 😊😊😊 894ae38 switches it so that we're just passing the L10N keys into $t(label) and $t(ariaLabel) :) I checked and all BtnLabeled instances have an ariaLabel. As we're always saying, there doubtless are some cases where there are some things missing, but I think that we're good to mark this one as closed πŸš€ If we find further things we can open new issues. I think those would be ideal good first issues for new contributors :) :)

Thanks for all your help here! Really heartening to have someone who's willing to go the distance to make sure that we're so accessible 😊

luuu-xu commented 1 year ago

@andrewtavis Thank you for the quick commit! I was actually doing the attributes reordering and styling changes and this ariaLabel change myself. But I don't have enough time to finish them! Anyways, I am glad we got these completed!! ❀️

andrewtavis commented 1 year ago

Sorry for the overlap or work, @luuu-xu! I wasn’t exactly sure what you were working on, so just get done what I could. Will be better from now on when we can have smaller issues :)