Skip to content
This repository has been archived by the owner on Jun 22, 2021. It is now read-only.

Added aria label support to markers via optional fn #531

Merged
merged 2 commits into from
Oct 17, 2019
Merged

Added aria label support to markers via optional fn #531

merged 2 commits into from
Oct 17, 2019

Conversation

crutchcorn
Copy link
Contributor

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:

  • Adds tabindexes to the map markers
  • Allows the consumer to add an aria label into the map marker based on a function that's passed in

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

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 14, 2019
@jpoehnelt jpoehnelt self-requested a review October 14, 2019 15:40
Copy link
Contributor

@jpoehnelt jpoehnelt left a 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.

packages/markerclustererplus/src/markerclusterer.js Outdated Show resolved Hide resolved
packages/markerclustererplus/src/markerclusterer.js Outdated Show resolved Hide resolved
@@ -738,6 +751,7 @@ class MarkerClusterer {
this.listeners_ = [];
this.activeMap_ = null;
this.ready_ = false;
this.ariaLabelFn = opt_options.ariaLabelFn || function () {return ''};
Copy link
Contributor

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?

Copy link
Contributor Author

@crutchcorn crutchcorn Oct 14, 2019

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 :)

Copy link
Contributor

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.

@crutchcorn
Copy link
Contributor Author

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 ''};
Copy link
Contributor

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.

@jpoehnelt jpoehnelt merged commit 03c1246 into googlemaps:master Oct 17, 2019
@crutchcorn
Copy link
Contributor Author

If this issue can be pinged when a version of it is released to NPM, that would be greatly appreciated

@jpoehnelt
Copy link
Contributor

published to: @google/[email protected]

the primary changes to 3.0.0 are packaging related

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants