website icon indicating copy to clipboard operation
website copied to clipboard

Rewriting syntax styles with styled-components

Open d-ivashchuk opened this issue 7 years ago • 21 comments

So I've done my best to rewire syntax but there are some issues, that need to be tackled:

  1. Responsive layout of ShowList component, it's currently broken for some reason
  2. Bars components does not stop on pause, fix would probably be passing the state to the component and pausing animation accordingly.
  3. Player component has troubles with layout
  4. Player component's scrabbing functionality is broken 5.Player component's loudness does not work as expected.

If anyone will be working on these fell free to ping me.

Thanks @wesbos for doing an amazing job!

Issue #157

d-ivashchuk avatar Oct 02 '18 18:10 d-ivashchuk

Hey @d-ivashchuk I can help out with this! Is there anything on that list you've already tackled?

bchiang7 avatar Oct 03 '18 02:10 bchiang7

Hey @bchiang7, nope, you can have a look at those small issues and try to fix them:)

d-ivashchuk avatar Oct 03 '18 07:10 d-ivashchuk

Almost done with this! Will push my updates tonight

bchiang7 avatar Oct 11 '18 13:10 bchiang7

@d-ivashchuk https://github.com/d-ivashchuk/Syntax/pull/2

bchiang7 avatar Oct 12 '18 03:10 bchiang7

@bchiang7 Merged your PR and I think it's good to go!

d-ivashchuk avatar Oct 12 '18 14:10 d-ivashchuk

@d-ivashchuk sweet, then I think you can remove the WIP in the PR title!

bchiang7 avatar Oct 12 '18 14:10 bchiang7

Note: Unrelated to styled components, but to make development easier I added ESLint and prettier configs from @wesbos dotfiles, so he should be familiar with it. Adding the ESLint config led to modifying things beyond styled components to resolve as many linting errors as possible (like prop types and a11y things), but these changes should not affect any functionality.

bchiang7 avatar Oct 12 '18 15:10 bchiang7

Well done @d-ivashchuk & @bchiang7. The size of the h2 in the show notes is a little smaller and some other little things I noticed.

There is some redundant code, for example, you could move all of the footer styles out of the styles folder and in to the footer component or use your wrapper. Look into withComponent in the styled components docs.

Also, named exports would have made your life a little easier in the styles folder. Just a couple things.

I think it looks good. It'll work out nicely 👍

ghost avatar Oct 14 '18 11:10 ghost

@d-ivashchuk @bchiang7 @jameygleason hey guys 👋🏼 Would you guys be able to collaborate on this one? I found a few duplicates of this feature and I want to circle it all back to the original one here. I left some comments on https://github.com/wesbos/Syntax/pull/186.

Also, @bchiang7 Brittany, I think a linter is a great idea! However, its a whole separate can of worms that would need to lint the whole application and probably deserves its own PR. I will merge as much as I can over the next few days and then it would be great to lint all that at once. How does that sound? 😃

sergical avatar Oct 22 '18 23:10 sergical

@416serg Fine by me!

@d-ivashchuk since this PR is using your fork it might be quicker for you to make modifications, but I can certainly submit PRs to your fork again as well

bchiang7 avatar Oct 22 '18 23:10 bchiang7

Hey @d-ivashchuk @bchiang7 anything I can help out with on this issue? There's a lot in this PR, not really sure what's been completed and what's left to do.

davejsdev avatar Oct 23 '18 00:10 davejsdev

@davidleger95 Hey David! There's definitely a few things you could do:

  1. Resolve the conflicts (rebase from master)
  2. Remove the .eslintrc and .prettierrc along with the devDependencies that were installed along with it:
    "babel-eslint": "^10.0.1",
    "eslint": "^5.6.1",
    "eslint-config-airbnb": "^17.1.0",
    "eslint-config-prettier": "^3.1.0",
    "eslint-plugin-import": "^2.14.0",
    "eslint-plugin-jsx-a11y": "^6.1.2",
    "eslint-plugin-prettier": "^3.0.0",
    "eslint-plugin-react": "^7.11.1",
    "prettier": "^1.14.3"

No need to remove babel-plugin-styled-components, that one's not for linting.

bchiang7 avatar Oct 23 '18 00:10 bchiang7

@bchiang7 @davidleger95 I went ahead and pushed https://github.com/wesbos/Syntax/pull/212 with an .eslintrc from @wesbos's https://github.com/wesbos/dotfiles/blob/master/.eslintrc

I haven't added anything to the package.json file since I have all the linters installed globally but maybe it would be a good idea to add them as devDependencies.

Thanks for all the hard work guys!

sergical avatar Oct 23 '18 01:10 sergical

@416serg Thanks! I definitely think you should add the devDependencies for linting, otherwise anyone who forks this project won't know to install them until they hit errors

@davidleger95 let me know if you wanted to pick those two things up, otherwise I can do it!

bchiang7 avatar Oct 23 '18 02:10 bchiang7

@bchiang7 Yeah I can do that. I'll probably have time to do that tomorrow evening, but if you get to them before me that's fine too. Just let me know if you start working on it so we don't both do the same work. 🙃

davejsdev avatar Oct 23 '18 03:10 davejsdev

@davidleger95 the merge conflict is pretty tricky, I took a stab at it but I think it would be easiest if @d-ivashchuk rebased his fork

bchiang7 avatar Oct 23 '18 04:10 bchiang7

Sorry, I didn’t participate in discussion due to my time zone. I could rebase it but I am not sure when I will be able to do it, for sure in the next couple of days!

d-ivashchuk avatar Oct 23 '18 05:10 d-ivashchuk

@d-ivashchuk I went through and resolved the merge conflicts, updated code is on the master branch of my fork https://github.com/bchiang7/Syntax/

Since your master branch is behind, we can't automatically merge my fork into your fork, which is unfortunate. There's a few ways we can make this rebase less of a headache:

  1. You manually update your fork by copying the code from my fork (replacing each of your files with the content from my files to avoid the work of deciding which bits to keep and which to scrap)
  2. You add me as a collaborator on your fork and I force push my code up to d-ivashchuk/master
  3. Create new PR with my fork (this would probably be the easiest thing to do, but I'd hate to take away any credit from you, especially if you're counting this PR for Hacktoberfest)

Let me know!

bchiang7 avatar Oct 23 '18 19:10 bchiang7

I think I'm going to hold off on working with this until the conflicts are resolved. @bchiang7 If you want, you can go ahead and remove the linter files and deps since I think you're more familiar with what state which forks/branches are in at the moment. There will always be more to do later on. :)

davejsdev avatar Oct 23 '18 20:10 davejsdev

Hey @bchiang7 ! I’ve added you as collaborator to my fork, but you are also free to create new pr with your fork, if force pushing is more complicated for you. Thanks for working on this one with me!

d-ivashchuk avatar Oct 24 '18 20:10 d-ivashchuk

@d-ivashchuk @416serg We're good to go!

bchiang7 avatar Oct 24 '18 23:10 bchiang7