-
Notifications
You must be signed in to change notification settings - Fork 36
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
Fix console warnings #12
Conversation
This PR fixes console warnings across the application by: - moving `class` to `className` - adding a `key` prop when looping through an array of items and outputting it to the DOM (see [the docs](https://reactjs.org/docs/lists-and-keys.html) for explanation on this) - moving `stop-opacity` and `stop-color` to `stopOpacity` and `stopColor`, respectively Closes #10
This div is currently hidden by CSS, but I've removed it in this commit because it currently embeds an `<a>` tag inside of _another_ `<a>` tag in the `Project` component, which is obviously not browser-compliant. This is a separate commit in case we want to revert this or do a different change to handle this particular console warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for doing this. I really appreciate it. I left a few comments on some minor things.
My only philosophical concern with the className
changes is I can imagine how it potentially makes it harder to share these templates with https://github.com/cloudflare/workers.cloudflare.com/blob/9f5d0345f3e29ffdfd1eca96af4342e83f91f18c/pages/index.html#L333-L378. My original vision was to take these static HTML strings and put them into https://github.com/ashleygwilliams/workers-brand-assets—hopefully move to @cloudflare by then ;) —and then import them as we do CSS into both projects. Of course this becomes moot if we Gatsby-ify workers.cloudflare.com or absorb it into this project. Anyhow, my point being: I’m fine with this change as long as we don’t lose sight of that long-term goal.
👍
{projects | ||
.filter( | ||
project => | ||
project.features.length && | ||
project.features.map(f => f.name).includes(feature.name) | ||
) | ||
.map(project => ( | ||
<div class="ProjectsRow--project"> | ||
<div className="ProjectsRow--project" key={project.name}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is project.name
guaranteed to be unique? (If not, is there something like project.id
we can use?) [2/2]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
absolutely, that's probably a better way of doing this. will update
@@ -13,29 +13,29 @@ const IndexPage = ({ data: { allSanityFeature, allSanityProject } }) => { | |||
|
|||
return ( | |||
<Layout> | |||
<SEO /> | |||
<SEO title="Homepage" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that this was just a placeholder, but let’s change this to "Built with Workers" for now instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, so this is an interesting one - the SEO component actually sets a "Built with Workers" base title. passing this in will render "Homepage | Built with Workers". maybe should open a separate ticket here, OR make this field optional. having the homepage render just "Built with Workers" makes sense, i think
<div class="ProjectsRow"> | ||
<div class="ProjectsRow--title"> | ||
<h2 class="ProjectsRow--title-content"> | ||
<div className="ProjectsRow" key={feature.name}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is project.name
guaranteed to be unique? (If not, is there something like project.id
we can use?) [1/2]
<linearGradient id="CloudflareWorkersLogoCombinationMarkHorizontal--gradient-a" x1="50%" x2="25.7%" y1="100%" y2="8.7%"> | ||
<stop offset="0%" stop-color="#eb6f07"/> | ||
<stop offset="100%" stop-color="#fab743"/> | ||
<linearGradient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to reformat these <linearGradient/>
’s like this? I personally find this style much harder to read for <svg/>
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reformatting meaning having the attributes on separate lines? this is my editor doing its thing, i can definitely disable it here. sorry about that!
@adamschwartz thanks for review! I'm going to look into the class v className thing and see if we can find a solution to that. |
@adamschwartz per facebook/react#10169, there doesn't seem to be any plans for React to support this. how would you feel about proceeding with |
I’m fine with switching to |
going to close this, so many merge conflicts that it would probably be more worthwhile to just open a new PR 💀 |
Thanks. Just pushed 6a937f7 to get us somewhere. |
This PR fixes console warnings across the application by:
class
toclassName
key
prop when looping through an array of items and outputting it to the DOM (see the docs for explanation on this)stop-opacity
andstop-color
tostopOpacity
andstopColor
, respectivelyCloses #10
In 6d5c261, I've removed the
Project--features
div from inside theProject
component. This div is currently hidden by CSS, but I've removed it from the component entirely in this commit because it currently embeds an<a>
tag inside of another<a>
tag, which is obviously not browser-compliant. I've made this a separate commit in case we want to revert this or do a different change to handle this particular console warning.