search

lifechurch / youversion-web-open-ideas

Ideas, problems, and puzzling things that are open for volunteer involvement. See the issues page for open ideas, and create a pull request if you have a solution!
https://www.youversion.com/volunteers/
Apache License 2.0
39 stars 18 forks source link

<VideoThumbnail /> Component #15 #27

Closed borozcod closed 6 years ago

borozcod commented 6 years ago
michael-martin-al commented 6 years ago

@borozcod do you have any questions about the feedback on your PR? It's look like it's very close to being ready for merge. Nice work!

borozcod commented 6 years ago

Hi @michael-martin-al, yes I do. Code climate seems to be complaining about the following:

Definition for rule 'jsx-a11y/anchor-is-valid' was not found import React from 'react'

Not sure what is going on here, it seems to be a standard import statement to me.

michael-martin-al commented 6 years ago

I don't think it's complaining about the import. Instead, I think it is about an invalid <a/> somewhere. This could be a problem extended from melos and one of our link components that you're importing. Essentially, the rule requires that anchor tags always have a valid href. If you're using a <Link /> component, there are ways to tweak the rule to look for a to attribute or whatever. https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/docs/rules/anchor-is-valid.md#anchor-is-valid If the problem turns out to be a melos problem, let's just report an issue there and you can ignore the warning.

borozcod commented 6 years ago

Hi @michael-martin-al I'm still not sure what is causing this problem. After seeing your notes, I looked into the Link component I included from react-router-dom. I replaced this component with the Link component from melos. This allowed me to add an href as a prop. Even after this change, I was not able to figure out the issue. Has this been something someone else has experienced? The only melos components I'm including are Link and LazyImage.

codeclimate issue: Definition for rule 'jsx-a11y/anchor-is-valid' was not found: import React from 'react'

tcg commented 6 years ago

Re: "Definition for rule ___ was not found": The issue appears to be with underlying dependencies or configuration. See: https://github.com/lifechurch/youversion-web-open-ideas/pull/29#issuecomment-382760456

We can ignore that for now. 🎉

borozcod commented 6 years ago

@tcg Sounds good! I removed "jsx-a11y/anchor-is-valid" from the .eslintrc.js file. This seemed to work :)

tcg commented 6 years ago

@borozcod It definitely gets CodeClimate to stop complaining -- but we still want that rule active for our local linters. It works locally. So for now, let's keep that in the ESLint conf file, but we'll just specifically ignore that particular CodeClimate issue.

I reached out to their support to see if it's a misconfiguration on our behalf, or something wonky with their default setup.

borozcod commented 6 years ago

@tcg sounds good. On my end should I revert my changes to .eslintrc.js and push another commit? Can I ignore the issue on code climate or is this something done on your end?

tcg commented 6 years ago

@borozcod Yes, please revert that particular change. I'll see if I can mute that alert on CC. Otherwise, we'll literally just overlook it. I haven't fully reviewed the rest, but we'll try to provide more feedback, or just get this merged soon. We've been pretty caught up today trying to stabilize some things on Bible.com 😊

borozcod commented 6 years ago

Ok sounds good! I pushed my commit reverting the change to the eslintrc.js file. Thank you for all your help @tcg !

tcg commented 6 years ago

I may have inadvertently caused some naming convention weirdness, so before I'm done reviewing here, I want to settle https://github.com/lifechurch/youversion-web-open-ideas/pull/30

borozcod commented 6 years ago

Hi, @tcg I noticed michael gave the thumbs up on cleaner import statements. Should I update the filenames just for the video thumbnail component?