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

chore(project): add anchor tags for better seo #345

Merged
merged 26 commits into from
Aug 3, 2023

Conversation

olga-jwp
Copy link
Collaborator

@olga-jwp olga-jwp commented Jul 18, 2023

Description

OWA-6

This PR solves # .

Steps completed:

  • Add tag inside Shelf.tsx

  • Remove onCardClick() from Shelflist.tsx

According to our definition of done, I have completed the following steps:

  • Acceptance criteria met
  • Unit tests added
  • Docs updated (including config and env variables)
  • Translations added
  • UX tested
  • Browsers / platforms tested
  • Rebased & ready to merge without conflicts
  • Reviewed own code

@github-actions
Copy link

github-actions bot commented Jul 18, 2023

Visit the preview URL for this PR (updated for commit b6c6df5):

https://ottwebapp--pr345-chore-seo-add-anchor-4rtiv4wy.web.app

(expires Fri, 01 Sep 2023 14:53:09 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c198f8a3a199ba8747819f7f1e45cf602b777529

@olga-jwp olga-jwp requested a review from dbudzins July 18, 2023 11:44
posterAspect={posterAspect}
item={item}
/>
<a href={isInView ? url : undefined} className={styles.cardWrapper}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is better to update the Card component because now it results in a button wrapped by an anchor tag. This negatively impacts the accessibility for keyboard/screenreader users. Besides that, the Card component is also used on the playlist screens, which use the CardGrid component.

Current HTML

<a href="/m/uB8aRnu6/agent-327?r=dGSUzs9o" class="_cardWrapper_1ge8j_66">
  <div class="_card_1lln7_1" tabindex="0" role="button" aria-label="Play Agent 327">

Expected HTML

<a href="/m/uB8aRnu6/agent-327?r=dGSUzs9o" class="_card_1lln7_1" aria-label="Agent 327">

I also see that the aria-label contains the prefix "Play " which can be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey Chris, thanks! I've discussed that with Danny, and I am indeed moving the tag inside of the Card component.

@olga-jwp olga-jwp changed the base branch from develop to feature/cumulative-seo-cards July 25, 2023 13:40
@olga-jwp olga-jwp merged commit a754f56 into feature/cumulative-seo-cards Aug 3, 2023
9 checks passed
@olga-jwp olga-jwp deleted the chore/seo-add-anchor-tag branch August 3, 2023 07:46
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