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

Feature/add support for custom image module with image alias #93

Conversation

FrederickEngelhardt
Copy link
Contributor

@FrederickEngelhardt FrederickEngelhardt commented Apr 18, 2020

In response to #92

Use case:

  • As a developer I want the option to alias Image and not run into A11y errors for Image components that already use A11y.

Example:

import Image from 'components/Image/Image'

const Component = (props: SomeProps) => (
<>
  <Image {...props} /> // could have props passing through or could be all defaulted inside components/Image/Image
</>
)

Sample image component

import { Image } from 'react-native'

const RNImage = (props) => (
  <Image source={props.source} /> // this component will throw the error for has-valid-accessibility-ignores-invert-colors
)

This PR also support module import linting

import * as RN from 'react-native'
const { Image } = RN

const Component = (props: ImageProps) => (
  <Image source={props.source} /> // should fail
)

This PR will not include resolutions for aliasing named Imports such as

import { Image as RNImage } from 'react-native'

const Component = (props: ImageProps) => (
  <RNImage source={props.source} /> // will not throw an error b/c we're looking for 'Image'
)

Perhaps this can be handled at a later date...

Proposal:

Adds ability to check for A11y for react-native Images and any Image aliased component that are identified. Otherwise the linter will not check and exists out early.

Changes:

  • Checks for Imports that resolve for react-native and image only otherwise ignores aliased Image components
  • Adds ability to alias tests (only for has-valid-accessibility-ignores-invert-colors so far) but the boilerplate is there for generation of these labels. To me, it feels much cleaner seeing these use cases cleanly identified in sentences rather than seeing large amounts of JSX. Following this structure you can now add a title to an it block instead of it becoming JSX.
  • Clean up a failing test from the master branch and add in tests for basic, ios, android, all options.

Demo Tutorial

Run the following commands

  1. rm -rf node_modules // not necessary but it might be beneficial
  2. yarn cache clean // essential
  3. yarn add --dev github:FrederickEngelhardt/eslint-plugin-react-native-a11y#v2.01-rc1 // this branch was rebased off this current branch with the lib/ files built using node 12 and committed to the branch.
  4. Confirm that importing a file aliased as Image will not throw any accessibilityIgnoresInvertColors errors unless it's imported from react-native

Troubleshooting

  • Noting that it was surprisingly time intensive and frustrating getting this repo working for debugging locally. I ended up updating the Readme with a quick tutorial for others.
  • Main issue was yarn cache, it was caching the previously installed npm module so local files and github repos failed to install correctly resulting in errors finding the plugin (which did exist in node_modules).
  • I also updated the .babelrc to use node 12. Node 4 seems a bit low for a react-native dependency. Thoughts?

Let me know what you think of this PR. Thanks!

@jpdriver
Copy link
Contributor

hey @FrederickEngelhardt -- thanks for the PR. although this is an edge case I like the overall concept here and I'd be happy to add support in.

right now while testing, it looks like there are a couple of issues that need to be addressed. not sure why this is no longer reported when it used to be before?

  • v2.0.0

Screenshot 2020-05-16 at 11 03 15

  • this branch

Screenshot 2020-05-16 at 11 02 09

also really sorry but after merging another PR first, you'll now need to rebase on top of latest master 😬

@FrederickEngelhardt FrederickEngelhardt force-pushed the feature/add-support-for-custom-image-module-with-image-alias branch from d86f125 to a2363e5 Compare May 1, 2021 02:10
@FrederickEngelhardt
Copy link
Contributor Author

FrederickEngelhardt commented May 1, 2021

@jpdriver Sorry for the delay. Ended up cherry-picking my changes and bringing them over from master.

This PR should be good. Those errors you were receiving should be resolved. Only thing to note is the module import syntax (which nobody should be doing now) is not supported.

Example of cases where the errors will not work.

import  * as RN from 'react-native
const { Image } = RN

// or
const RN = require('react-native')
const {Image} = RN

Otherwise aliases such as RNImage or anything you name it as from react-native lib will work.

import React from 'react'
import { Image as RNImage } from 'react-native'

const CustomImage = () => {
  return <RNImage />
}

export default CustomImage

@zibs zibs force-pushed the feature/add-support-for-custom-image-module-with-image-alias branch from 078c879 to 40b806f Compare September 9, 2024 22:24
@zibs zibs self-requested a review September 10, 2024 21:02
Copy link
Contributor

@zibs zibs left a comment

Choose a reason for hiding this comment

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

Thanks!

@carbonrobot carbonrobot merged commit a688a55 into FormidableLabs:master Sep 11, 2024
1 check passed
@github-actions github-actions bot mentioned this pull request Sep 11, 2024
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.

4 participants