Skip to content

Conversation

@dleroux
Copy link
Contributor

@dleroux dleroux commented Sep 9, 2019

WHY are these changes introduced?

Currently, when an accessibilityLabel is not provided to the Resource List the overlaying Anchor has no content which doesn't meet our accessibility requirement

"Anchor element found with a valid href attribute, but no link content has been supplied."

WHAT is this pull request doing?

Provides a value but either outputting the specified accessibilityLabel if none a is provided text that will include the name prop, or the resourceName, whichever is provided. For example

View details for Mae Jemison OR View details for customer

This change will also allow us to remove the a11y_shitlist 🎉

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

@dleroux dleroux force-pushed the removing-shitlist branch 5 times, most recently from abfb0d4 to 669fdda Compare September 12, 2019 13:10
@dleroux dleroux changed the title [WIP][ResourceItem] - add default accessibility label [WIP][ResourceItem] - Add default accessibility label and remove a11y_shitlist Sep 12, 2019
@dleroux dleroux changed the title [WIP][ResourceItem] - Add default accessibility label and remove a11y_shitlist [ResourceItem] - Add default accessibility label and remove a11y_shitlist Sep 12, 2019
@dleroux
Copy link
Contributor Author

dleroux commented Sep 12, 2019

@amrocha with this change we can remove all the a11y_shitlist! The tests were all based on the shitlist. Any suggestion on testing the actual script or do you feel we are good without it?

Codecov here is not showing anything useful that I can see.

Copy link
Contributor

@amrocha amrocha left a comment

Choose a reason for hiding this comment

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

I love it, thank you so much for taking this on!

@amrocha
Copy link
Contributor

amrocha commented Sep 12, 2019

@dleroux I think we're fine to delete the tests, the script is simple enough that I'm happy with it as is

@amrocha
Copy link
Contributor

amrocha commented Sep 12, 2019

@dpersing Hey Devon check this out

Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

Aww yes, this is a fantastic milestone for accessibility stuff!

@dleroux dleroux merged commit 56f543d into master Sep 12, 2019
@dleroux dleroux deleted the removing-shitlist branch September 12, 2019 22:04
@dleroux dleroux temporarily deployed to production September 20, 2019 17:44 Inactive
@dleroux dleroux temporarily deployed to production September 23, 2019 15:28 Inactive
@dleroux dleroux temporarily deployed to production September 23, 2019 15:38 Inactive
@dleroux dleroux temporarily deployed to production September 23, 2019 16:59 Inactive
@dleroux dleroux temporarily deployed to production September 23, 2019 17:05 Inactive
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