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

Adds alt text for images on landing page #3285

Merged
merged 10 commits into from
Sep 7, 2023
Merged

Conversation

kevinjxc
Copy link
Collaborator

@kevinjxc kevinjxc commented Jul 7, 2023

Resolves #2522

Add alt text for images

@kevinjxc kevinjxc requested a review from misaugstad July 7, 2023 18:51
Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

I added 2 comments in the code. Could you also look for alt text for any other images that we already have that isn't very helpful and update them (I'm thinking about the collaborator logos). Those logos have alt text like "Makeability_Logo" which isn't very helpful!

app/views/index.scala.html Outdated Show resolved Hide resolved
conf/messages Outdated
@@ -3,4 +3,4 @@ error.required = This field is required
invalid.credentials = Invalid credentials!
access.denied = Access denied!
user.exists = There already exists a user with this email!
could.not.authenticate = Could not authenticate with social provider! Please try again!
could.not.authenticate = Could not authenticate with social provider! Please try again!
Copy link
Member

Choose a reason for hiding this comment

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

Make sure to keep newlines at the ends of files!

@misaugstad
Copy link
Member

Can you clean up the PR a bit more before I review again? There are translations in the en-NZ file that shouldn't be there, and not all of the new images with alt text have the text in translations files.

Apologies if I looked before you had finished updating the PR. When you're done making updates and the a PR is ready for another review, please post a message on that PR like this so that I know that you're done!

@misaugstad
Copy link
Member

Also, the messages.zh files has been renamed to messages.zh-TW, so there is a conflict to resolve, unfortunately!

@kevinjxc
Copy link
Collaborator Author

Ready for review: my edits are split between #3358 and this PR.

#3358 is where I edited index.scala.html to have @Messages in order to translate.

#2522 (here) is where I edited all of the conf/messages.[LANGUAGE CODE] to hold the translations.

@kevinjxc kevinjxc requested a review from misaugstad August 17, 2023 01:14
@misaugstad
Copy link
Member

Can you resolve merge conflicts before I review? I suppose I should wait until after the People Nudge logo PR has been merged as well, but merge conflicts are going to need to be fixed at some point anyway!

@misaugstad
Copy link
Member

It shouldn't be too bad. Really all that we did was rename "messages.zh" to "messages.zh-TW" and Github doesn't know how to handle that automatically.

@kevinjxc kevinjxc force-pushed the 2522-alt-text-for-images branch 2 times, most recently from 861910d to 9cc656a Compare August 19, 2023 05:47
Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

We should probably add alt text for these logos in the footer as well!
Screenshot from 2023-08-29 15-36-07

app/views/index.scala.html Outdated Show resolved Hide resolved
app/views/index.scala.html Outdated Show resolved Hide resolved
app/views/index.scala.html Outdated Show resolved Hide resolved
app/views/index.scala.html Outdated Show resolved Hide resolved
conf/messages.en Outdated Show resolved Hide resolved
Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

Looks good! I did make a couple changes myself, listed below:

  • As per style guide, make sure that your files end with newlines. When you added to the messages files, you removed the blank lines at the ends of files.
  • We only need to include a "en-NZ" translation if it's different from "en". As such, I was able to remove all the translations you added to that file.

@misaugstad misaugstad changed the title 2522 alt text for images Adds alt text for images on landing page Sep 7, 2023
@misaugstad misaugstad merged commit 4df1a10 into develop Sep 7, 2023
@misaugstad misaugstad deleted the 2522-alt-text-for-images branch September 7, 2023 22:48
@misaugstad misaugstad mentioned this pull request Sep 29, 2023
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.

Add alt-text for more images
2 participants