-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Added aria label support to markers via optional fn #531
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just some minor comments.
@@ -738,6 +751,7 @@ class MarkerClusterer { | |||
this.listeners_ = []; | |||
this.activeMap_ = null; | |||
this.ready_ = false; | |||
this.ariaLabelFn = opt_options.ariaLabelFn || function () {return ''}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to default to something else? may be just pass the text value through?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what we could safely default it to without making some assumptions about the application. If we hard-code a value, we assume the user's language. If we hard-code a specific messaging about places, it assumes the type of content that's being clustered, etc.
I otherwise don't mind what we default to, assuming there's a consensus or word on what we want to do :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave it as is for now.
Co-Authored-By: Justin Poehnelt <[email protected]>
Has there been any discussion internally on what we want to do for the default function @jpoehnelt ? |
@@ -738,6 +751,7 @@ class MarkerClusterer { | |||
this.listeners_ = []; | |||
this.activeMap_ = null; | |||
this.ready_ = false; | |||
this.ariaLabelFn = opt_options.ariaLabelFn || function () {return ''}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave it as is for now.
If this issue can be pinged when a version of it is released to NPM, that would be greatly appreciated |
published to: the primary changes to 3.0.0 are packaging related |
For one of the projects I've been working on lately, we wanted to make sure our map marker implementation was interactable for A11Y. Part of this required us to make changes to this file. However, I did not realize the file was copy-pasted from upstream. So I'm hoping to mainline this change and change what we need to in order to make this upstreaming possible.
The following PR makes the following changes:
Again, I developed this solution without a broader understanding of this library, please let me know what changes I can make on the design side if there are any to help get this functionality in