Skip to content

🚸 UX: Use anchor tags in article-meta/basic.html#2252

Merged
nunocoracao merged 2 commits intonunocoracao:devfrom
servedsmart:replace-custom-javascript-logic-with-anchor-tags
Jul 17, 2025
Merged

🚸 UX: Use anchor tags in article-meta/basic.html#2252
nunocoracao merged 2 commits intonunocoracao:devfrom
servedsmart:replace-custom-javascript-logic-with-anchor-tags

Conversation

@servedsmart
Copy link
Copy Markdown
Contributor

@servedsmart servedsmart commented Jun 18, 2025

  • This removes an inline event handler
  • This also adds an aria-label

Purpose

Using javascript instead of just using <a> tag is bad practice regarding accessibility and automated processing of web pages. Even the browser doesn't show a hint if hovering badges when using the old implementation. This in my opinion is a good way to replace inline event handlers in that code.

Notes

Previously, this was part of #2231 and #2218, but after some thinking, I just split up the PR.

Warning

⚠️ This currently isn't compatible with #2249 and #2268. Please notify me to make the necessary changes.

@netlify
Copy link
Copy Markdown

netlify bot commented Jun 18, 2025

Deploy Preview for snazzy-dango-efb2ec ready!

Name Link
🔨 Latest commit 5d6b034
🔍 Latest deploy log https://app.netlify.com/projects/snazzy-dango-efb2ec/deploys/685328ac006de30008002d03
😎 Deploy Preview https://deploy-preview-2252--snazzy-dango-efb2ec.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@servedsmart servedsmart changed the base branch from main to dev June 27, 2025 17:14
@servedsmart servedsmart force-pushed the replace-custom-javascript-logic-with-anchor-tags branch from 5d6b034 to cfc92bf Compare June 27, 2025 17:24
@servedsmart servedsmart force-pushed the replace-custom-javascript-logic-with-anchor-tags branch from cfc92bf to 844b353 Compare July 9, 2025 17:13
@servedsmart

This comment was marked as outdated.

@nunocoracao
Copy link
Copy Markdown
Owner

@servedsmart conflicts here - can you have a look?

@nunocoracao nunocoracao deleted the branch nunocoracao:dev July 10, 2025 11:22
@nunocoracao nunocoracao reopened this Jul 10, 2025
@servedsmart servedsmart force-pushed the replace-custom-javascript-logic-with-anchor-tags branch from 844b353 to e19195d Compare July 10, 2025 13:52
@servedsmart

This comment was marked as outdated.

@nunocoracao
Copy link
Copy Markdown
Owner

@servedsmart conflicts again - sorry

@nunocoracao nunocoracao deleted the branch nunocoracao:dev July 11, 2025 09:36
@nunocoracao nunocoracao reopened this Jul 11, 2025
- This removes an inline event handler
- This also adds an aria-label

Using javascript instead of just using <a></a> is bad practice regarding accessibility and automated processing of web pages. Even the browser doesn't show a hint if hovering badges when using the old implementation. This in my opinion is a good way to replace inline event handlers in that code.
@servedsmart servedsmart force-pushed the replace-custom-javascript-logic-with-anchor-tags branch from e19195d to b73c64f Compare July 11, 2025 15:08
@servedsmart
Copy link
Copy Markdown
Contributor Author

@servedsmart conflicts again - sorry

Thank you for notifying me and that's not a problem.

ℹ️ This has been tested on 2c71bda and is working just fine.

@nunocoracao nunocoracao merged commit 24cb579 into nunocoracao:dev Jul 17, 2025
1 check passed
@ZhenShuo2021
Copy link
Copy Markdown
Contributor

Hi @servedsmart

Hope you’re doing well!

I noticed that after @nunocoracao merged this commit, it seems to have caused some issues with the article links:

  1. The underline below the title is missing
  2. The article shortcode is broken

To double-check, I tried reverting the following commits:
git revert b73c64f4f53bbf74ad1b4f1d332b772445bdeba6
git revert 5e8561d83b1ffddf8401ee1c38997156303a5c0f
After reverting, everything appeared to work properly again, which makes me think these changes might be the cause.

Whenever you have a chance, could you please take a look? Thanks a lot!

@servedsmart
Copy link
Copy Markdown
Contributor Author

Hi @servedsmart

Hope you’re doing well!

I noticed that after @nunocoracao merged this commit, it seems to have caused some issues with the article links:

1. The underline below the title is missing

2. The `article` shortcode is broken

To double-check, I tried reverting the following commits: git revert b73c64f4f53bbf74ad1b4f1d332b772445bdeba6 git revert 5e8561d83b1ffddf8401ee1c38997156303a5c0f After reverting, everything appeared to work properly again, which makes me think these changes might be the cause.

Whenever you have a chance, could you please take a look? Thanks a lot!

Regarding 2.. This will take some work, I'm not sure how that has happened. See: https://deploy-preview-2252--snazzy-dango-efb2ec.netlify.app/docs/shortcodes/ There it works, that was my original deploy preview. It probably messed up because of other changes, I will resolve this ASAP.

Regarding 1. It seems like I didn't notice that, thank you very much for telling me. This will also be fixed.

@servedsmart

This comment was marked as resolved.

@servedsmart

This comment was marked as resolved.

@servedsmart
Copy link
Copy Markdown
Contributor Author

@ZhenShuo2021 I fixed all mentioned issues in #2351.

I did some testing and currently don't have a reason to think that this introduced additional bugs, I hope that this hotfix is working correctly now.

@ZhenShuo2021
Copy link
Copy Markdown
Contributor

@servedsmart thank you very much for your quick fix!

nunocoracao added a commit that referenced this pull request Aug 3, 2025
…ogic-with-anchor-tags

🚑 Hotfix for issues in #2252
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