Skip to content

General fixing#62

Merged
DiogoRDuarte merged 9 commits into
developfrom
dd-general-fixing
Sep 12, 2025
Merged

General fixing#62
DiogoRDuarte merged 9 commits into
developfrom
dd-general-fixing

Conversation

@DiogoRDuarte

@DiogoRDuarte DiogoRDuarte commented Sep 8, 2025

Copy link
Copy Markdown
Contributor

In this PR:

  • The context prop was moved inside the autoDescriptions.
  • The role="application" was added, allowing arrow navigation without the need to switch to focus mode.
  • Added arrow navigation was added to the shortcut guide, making it easier to navigate.
  • Arrow icons were replaced with text strings on the ShortcutGuide, ensuring they are read.
  • The <dl> structure of the ShortcutGuide was redone.
  • An unused CustomTranslations type import was fixed.

@DiogoRDuarte DiogoRDuarte linked an issue Sep 10, 2025 that may be closed by this pull request
@DiogoRDuarte DiogoRDuarte linked an issue Sep 11, 2025 that may be closed by this pull request
@DiogoRDuarte

Copy link
Copy Markdown
Contributor Author

The addition of the role="application" requires further validation with NVDA

>
{t(section.title)}
</h3>
<div

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The correct way to work with <dl> tags: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/dl
How its ok to use <div>s without breaking the <dl> logic: https://html.spec.whatwg.org/multipage/grouping-content.html#the-dl-element

Comment thread README.md
Comment thread CHANGELOG.md
Comment on lines +19 to +20
- Replaced arrow symbols with text on the ShortcutGuide
- Re-did ShortcutGuide `<dl>` structure

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would move this two to the ### Changed section.

Comment thread CHANGELOG.md Outdated
Comment on lines +34 to +37
let nextIndex = currentRowIndex + 1;
if (currentRowIndex === -1) {
nextIndex = 0;
}

@joaopalmeiro joaopalmeiro Sep 12, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Personal opinion: I find a ternary operator more readable for this case (and for the next function as well). For example:

const nextIndex = currentRowIndex === -1 ? 0 : currentRowIndex + 1;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

agreed!

Comment thread src/components/navigation/GuideKeyHandler.ts Outdated
);
if (rows.length === 0) return;

const activeElement = document.activeElement as HTMLElement | null;

@joaopalmeiro joaopalmeiro Sep 12, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think there's no need to cast document.activeElement. If there's no specific reason to do so, I would remove as HTMLElement | null.

Same for the previous function.

Comment thread src/components/navigation/GuideKeyHandler.ts
@joaopalmeiro

Copy link
Copy Markdown
Member

While testing the shortcut guide, I noticed that VoiceOver doesn't read question marks (?) and reads Esc as if it were an acronym. I've been reading a bit about this but can't find any definitive best practices. I would consider adding aria-labels for these symbols, for example:

<span aria-label="Question mark">?</span>
<span aria-label="Escape key">Esc</span>

I'm not sure what the best practice is here, but I'd say we should at least consider these changes for future work.

@DiogoRDuarte

Copy link
Copy Markdown
Contributor Author

While testing the shortcut guide, I noticed that VoiceOver doesn't read question marks (?) and reads Esc as if it were an acronym. I've been reading a bit about this but can't find any definitive best practices. I would consider adding aria-labels for these symbols, for example:

<span aria-label="Question mark">?</span>
<span aria-label="Escape key">Esc</span>

I'm not sure what the best practice is here, but I'd say we should at least consider these changes for future work.

While testing the shortcut guide, I noticed that VoiceOver doesn't read question marks (?) and reads Esc as if it were an acronym. I've been reading a bit about this but can't find any definitive best practices. I would consider adding aria-labels for these symbols, for example:

<span aria-label="Question mark">?</span>
<span aria-label="Escape key">Esc</span>

I'm not sure what the best practice is here, but I'd say we should at least consider these changes for future work.

The approach needs to be slightly different. The shortcuts are built from an object, so adding aria labels different from the shortcut displayed required extra steps for the dev. Instead, I will just replace it directly just like I did with the arrow symbols. WDYT?

@joaopalmeiro

Copy link
Copy Markdown
Member

While testing the shortcut guide, I noticed that VoiceOver doesn't read question marks (?) and reads Esc as if it were an acronym. I've been reading a bit about this but can't find any definitive best practices. I would consider adding aria-labels for these symbols, for example:

<span aria-label="Question mark">?</span>
<span aria-label="Escape key">Esc</span>

I'm not sure what the best practice is here, but I'd say we should at least consider these changes for future work.

While testing the shortcut guide, I noticed that VoiceOver doesn't read question marks (?) and reads Esc as if it were an acronym. I've been reading a bit about this but can't find any definitive best practices. I would consider adding aria-labels for these symbols, for example:

<span aria-label="Question mark">?</span>
<span aria-label="Escape key">Esc</span>

I'm not sure what the best practice is here, but I'd say we should at least consider these changes for future work.

The approach needs to be slightly different. The shortcuts are built from an object, so adding aria labels different from the shortcut displayed required extra steps for the dev. Instead, I will just replace it directly just like I did with the arrow symbols. WDYT?

As long as the labels don't get too big, that seems like a good approach!

@DiogoRDuarte DiogoRDuarte marked this pull request as ready for review September 12, 2025 16:58
@DiogoRDuarte DiogoRDuarte merged commit 8532c80 into develop Sep 12, 2025
4 checks passed
@DiogoRDuarte DiogoRDuarte deleted the dd-general-fixing branch September 12, 2025 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants