search

austintackaberry / ydkjs-exercises

Exercises to go along with You Don't Know JavaScript
MIT License
253 stars 74 forks source link

Many of our style rules are inlined #193

Open rreubenreyes opened 6 years ago

rreubenreyes commented 6 years ago

For scalability purposes, especially if we're making significant changes to the design, I think it would be best to avoid inline styling where possible, and instead make style declarations inside our styled components for component-specific styles, or in App.css for global rules.

esthercuan commented 5 years ago

I can take this one! Do you want a PR with ALL inline styles replaced? :) @radotreyes @austintackaberry

rreubenreyes commented 5 years ago

@esthercuan yeah, I think it would be best to replace all inline styles. But don't feel pressured to do it all in one PR, definitely feel free to break it up into multiple PRs or keep one big PR as a WIP for a while if you're short on time!

esthercuan commented 5 years ago

Ill go ahead and take a look at how many inline styles we have and let you know! :)

esthercuan commented 5 years ago

@radotreyes I have created a PR for this but tests are failing for unrelated issues. :( https://github.com/austintackaberry/ydkjs-exercises/pull/211

austintackaberry commented 5 years ago

@esthercuan Unfortunately, almost all of our tests are shallow renders which means that any time we change the JSX for each component, there is a risk of our tests failing.

Can you fix the tests based on your changes or do you want help?

esthercuan commented 5 years ago

I tried runing jest -u to update snapshots but it does not seem to do much. I also pulled master and ran jest, and tests are failing there too . hmmmm

austintackaberry commented 5 years ago

Yeah, that's expected. They are breaking as a result of changing all the styles from inline to styled components. The components are being shallowly rendered, so if you change a <h3> to <Header3>, then the tests don't know that they need to look for <Header3> instead of <h3>.

So in order to fix them, you need to change the tests to look for <Header3> instead of <h3>

esthercuan commented 5 years ago

Ok, Ill go ahead and fix them :) thanks for the tips!

austintackaberry commented 5 years ago

No problem!