Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

ensure map areas have non-empty alts #554

Closed
wants to merge 4 commits into from
Closed

ensure map areas have non-empty alts #554

wants to merge 4 commits into from

Conversation

Chovin
Copy link

@Chovin Chovin commented Oct 14, 2018

closes #223

@msftclas
Copy link

msftclas commented Oct 14, 2018

CLA assistant check
All CLA requirements met.

@Chovin
Copy link
Author

Chovin commented Oct 14, 2018

hmm, if this is is typescript, the only time i’d be able to tell if the area element is in a map element is if they’re both there together in their “literal” form right?

@JoshuaKGoldberg
Copy link

if they're both there together in their "literal" form

Sorry @Chovin I don't follow. Do you mean that the only way to tell is if the literals are together, like this?

<map ...>
    <area ... />
</map>

...rather than:

<SomeComponentThatReturnsAMap>
    <area ... />
</SomeComponentThatReturnsAMap>

If that's the case, then yes - there isn't a (known?) good way to validate what JSX elements are returned by a method.

Copy link

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Looks like you have a build failure:

src/reactA11yImgHasAltRule.ts(87,51): error TS2339: Property 'parentNode' does not exist on type 'JsxOpeningLikeElement'.
  Property 'parentNode' does not exist on type 'JsxSelfClosingElement'.

Are you missing a cast somewhere, or is that meant to be node.parent?

https://ci.appveyor.com/project/HamletDRC/tslint-microsoft-contrib/builds/19493473/job/jxjskojh6r8pqd5j

@Chovin
Copy link
Author

Chovin commented Oct 15, 2018

@JoshuaKGoldberg ah I see. I'm assuming that's the best we can do here and just ignore cases like this?

<SomeComponentThatReturnsAMap>
    <area ... />
</SomeComponentThatReturnsAMap>

@Chovin
Copy link
Author

Chovin commented Oct 15, 2018

hmm. parent.type === 'map' ?
Sorry, this is my first time touching React 🙃

Copy link

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

This looks like it should work, super. Let's add some tests to ReactA11yImgHasAltRuleTests.

@Chovin
Copy link
Author

Chovin commented Oct 15, 2018

Awesome! I’ll get to that test soon. Thanks!

@JoshuaKGoldberg JoshuaKGoldberg added the PR: Waiting for Author Changes have been requested that the pull request author should address. label Oct 26, 2018
@JoshuaKGoldberg JoshuaKGoldberg added the PR: Merge Target Branch Merge conflicts exist, but no other blockers to merging. label Nov 5, 2018
@JoshuaKGoldberg
Copy link

Ping @Chovin, are you still available to finish up this PR? Looks like it'd be a good one to have in.

@Chovin
Copy link
Author

Chovin commented Nov 20, 2018

I’d like to get back to this but will have to do so in a few weeks. Thanks for following up @JoshuaKGoldberg !

@JoshuaKGoldberg
Copy link

Closing now to keep the queue small so others feel welcome to take this on themselves. Feel free to ping me if you'd like to pick this back up @Chovin!

@IllusionMH IllusionMH added this to the 6.1.0-beta milestone Feb 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR: Merge Target Branch Merge conflicts exist, but no other blockers to merging. PR: Waiting for Author Changes have been requested that the pull request author should address.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update react-a11y-img-has-alt - area elements of image maps have alternate text
4 participants