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

Styled components #34

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sudarshang
Copy link

This PR is to get feedback on the right way to migrate this project to styled-components. I have only migrated the Video component for now. @art1fa please let me know if this is how #25 should be handled.

{tweets.map((t, i) => (
<Tweet autoPlay={true} data={t} key={i} linkProps={linkProps} />
))}
const filteredTweets = tweets.filter(tweet => {
Copy link
Author

Choose a reason for hiding this comment

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

I filtered the tweets out here to ease the testing of the styled component migration of Video.js.

@zebra-ok
Copy link
Contributor

Yeah looks fine :)
Just two comments:

  • It probably makes sense to organize the styles in another place than the actual component. This is one example, but of course there's more than one way...
  • Move the dependency in devDependencies and peerDependencies (see this)

@mannynotfound
Copy link
Owner

hey guys, thanks for the PR! Was out of town all weekend so havent gotten a chance to look at this but will provide some feedback soon.

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.

3 participants