website
website copied to clipboard
Rewriting syntax styles with styled-components
So I've done my best to rewire syntax but there are some issues, that need to be tackled:
- Responsive layout of
ShowListcomponent, it's currently broken for some reason Barscomponents does not stop on pause, fix would probably be passing the state to the component and pausing animation accordingly.Playercomponent has troubles with layoutPlayercomponent's scrabbing functionality is broken 5.Playercomponent'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
Hey @d-ivashchuk I can help out with this! Is there anything on that list you've already tackled?
Hey @bchiang7, nope, you can have a look at those small issues and try to fix them:)
Almost done with this! Will push my updates tonight
@d-ivashchuk https://github.com/d-ivashchuk/Syntax/pull/2
@bchiang7 Merged your PR and I think it's good to go!
@d-ivashchuk sweet, then I think you can remove the WIP in the PR title!
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.
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 👍
@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? 😃
@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
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.
@davidleger95 Hey David! There's definitely a few things you could do:
- Resolve the conflicts (rebase from master)
- Remove the
.eslintrcand.prettierrcalong with thedevDependenciesthat 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 @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!
@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 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. 🙃
@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
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 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:
- 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)
- You add me as a collaborator on your fork and I force push my code up to
d-ivashchuk/master - 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!
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. :)
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 @416serg We're good to go!