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

Add ability to customize navigation announcer #966

Closed
wants to merge 4 commits into from

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Apr 11, 2021

This introduces a new $app/a11y, which for now only exports setNavigationAnnouncer, which makes i possible to customize the navigation announcer, which should also solve the internationalization-part of it. The default stays untouched - which could be changed. It also could be enhanced such that one could turn it off if one returns null - up for discussion.
Closes #879

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts

This introduces a new `$app/a11y`, which for now only exports `setNavigationAnnouncer`.
Closes #879
@@ -169,6 +169,9 @@ function generate_app(manifest_data, base) {
let navigated = false;
let title = null;

$: navigationAnnouncer = stores.navigationAnnouncer;
$: navigationAnnouncement = $navigationAnnouncer.length > 0 ? $navigationAnnouncer[$navigationAnnouncer.length - 1](title) : ('Navigated to ' + title);
Copy link
Member Author

@dummdidumm dummdidumm Apr 11, 2021

Choose a reason for hiding this comment

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

Should this be try {} catch {}ed?

@Conduitry
Copy link
Member

Is this the best API for something like this? It feels a little weird to me. Could we instead have another file (similar to src/hooks.js) where you do export function navigationAnnouncer() { }, and that overrides the default one? I'm worried about this API being overly imperative rather than declarative.

@dummdidumm
Copy link
Member Author

I chose to not put it in one place because when you want different announcements for different routes and you'd need to model that in one place, it could get awkward quickly, also colocation would be lost. Another thing is that internationalization is probably easier to add if you have it within your .svelte files because you can reuse the same mechanism.

@Conduitry
Copy link
Member

Hm, okay. What about having it be a new <script context='module'> export from the .svelte file, or having it be a new thing in the response from the load function? I do agree that this is something that needs to be configurable, but I'm concerned about rushing into an API we'll regret, or that this will end up being one weird orphaned thing that nothing else in Kit behaves similarly to.

@dummdidumm
Copy link
Member Author

dummdidumm commented Apr 11, 2021

Making this an export from context="module" feels strange to me at first, I reserved those kinds of exports to SvelteKit render options in my mind.
Returning it from the load function feels like mixing concerns to me. Then again, the concerns are already pretty mixed in that return object, so yeah that could be a good alternative. What would be returned though, a function? If not, how would you access the document title, especially if it's inside <svelte:head> and translated? Also how to pass these up to root.svelte? Currently the semantic is that the returned object is passed down, not up.

Totally get your concerns about not rushing this. One half of this PR for me is to get the discussion rolling, the other half was to get to know the SvelteKit codebase a little 😄

@benmccann
Copy link
Member

Out of the options raised I think the current implementation or making it an export from context="module" make the most sense to me. I think I'd lean towards the export from context="module" option

dummdidumm and others added 2 commits April 12, 2021 18:55
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@Rich-Harris
Copy link
Member

closing in favour of #1305, per yesterday's discussion

@Rich-Harris Rich-Harris closed this May 2, 2021
@dummdidumm dummdidumm deleted the navigation-announcer branch May 2, 2021 16:06
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.

Make svelte-announcer text configurable
4 participants