RyanRussell00 / personal-dj

Personal DJ site that creates playlists given custom inputs. View it live at: http://personaldj.net/
http://personaldj.net/
MIT License
25 stars 12 forks source link

Added loader to the buttons #24

Closed astriskit closed 4 years ago

astriskit commented 4 years ago

Addressing #1

astriskit commented 4 years ago

@RyanRussell00 - let me know, if you face any issues or have anything to discuss about the PR.

RyanRussell00 commented 4 years ago

For future PR please don't prettify because that makes it much harder to review changes and see what files/lines you've changed. If you'd like to prettify just open a new PR with the prettified files, but for this PR that's fine.

It looks really good! Thank you for this contribution :)

astriskit commented 4 years ago

@RyanRussell00

For future PR please don't prettify because that makes it much harder to review changes and see what files/lines you've changed. If you'd like to prettify just open a new PR with the prettified files, but for this PR that's fine.

I'd suggest to have a prettier as a dependency, run repo through prettier and update the repo; I think, it'd add to the longevity of the project; Off-course if you find it fine, you can use git-hook/pre-commit to run prettier before any-one commits!

It looks really good! Thank you for this contribution :)

you are welcome and thanks!!

RyanRussell00 commented 4 years ago

@astriskit Yea this project blew up much faster and bigger than I thought it would. Hence the lack of build pipelines and coding style. Thank you for this suggestion, I'll get it implemented as soon as I get the chance. Thanks again!

RyanRussell00 commented 4 years ago

@astriskit I just added a GitHub action to the master branch where it will automatically run Prettier on any pushes to master and Pull Requests. Let me know if it's working for you or if I should change it up in any way

astriskit commented 4 years ago

@RyanRussell00 - will update you in a while!

astriskit commented 4 years ago

Update on pipeline/prettier :

@RyanRussell00 - I synced the fork with this/original repo and github-action ran; But, the prettier had not run successfully - 403 on the repo it is running - seemingly denying the write action on the repo

Screenshot 2020-10-07 at 8 43 25 AM

Also looking at the action-logs in this/original repo - seemingly the same issue.

On this note, I'd suggest to use prettier using on git-hooks. It'll make the future-PRs better formatted to read; Btw earlier I suggested for a one-time action to get the whole repo run through the prettier to start;

May be a nicer flow would be you get git-hooks configured on a local-master-synced-separate-branch -> run the entire-repo using prettier-cli -> commit (would also test the hook-running) -> push-to-merge-to-master-remote;

RyanRussell00 commented 4 years ago

@astriskit Okay thanks for that. I've never used git hooks before but I'll look into it and figure it out. In the meantime, I've removed the Prettify Action for PR and it will only apply to pushes to master; I'll remove it completely if it becomes too much of an issue.

astriskit commented 4 years ago

@RyanRussell00 - if you are fine with it, I'd add git-hook setup to run prettier before committing;

RyanRussell00 commented 4 years ago

@astriskit That's totally fine by me! I'll look into what git-hooks are and see what I can contribute as well. Feel free to leave a PR open as you work on it

astriskit commented 4 years ago

@RyanRussell00 - to start with I'll make an issue around that