Seneral / FlagPlayer

Music-oriented YouTube web-app - single-page, client-side, simple
https://flagplayer.seneral.dev
17 stars 5 forks source link

Fixed a bug that causes some keyword to act like a video url #11

Closed SezerYardim closed 2 years ago

SezerYardim commented 2 years ago

Closes #10

Some keyword ( spesifically ones with 11 length ) was acting like they are video urls instead of search keywords. I removed the line that controls whether the search bar value is a video url or not via regular expression as we talked about it in #10

Seneral commented 2 years ago

Thanks for your contribution! Glad to see some interest I'll need you to do two things before I can merge though: First, only remove the last regex for video ids, still need the others for full URLs to work Second, only one change at a time (or at least only documented ones), e.g. no unrelated changes

SezerYardim commented 2 years ago

Sorry it was my bad. I thought you want me to remove that feature completely. Anyways, i did the things you mentioned but i made a mistake and reset my local repo to the last commit before mine. After that i pulled it again from my fork, changed the things and committed again.

This is my first open source contribution. I hope i did not mess the things up. Incase i will close my PR back and do it again from the beginning.

Seneral commented 2 years ago

No worries, in the end I can squash merge so it appears as one commit. What counts is the total changes (best seen in github UI). That still includes the unwanted package lock changes, btw. Probablly happens because you run the server from the repo folder, so what I did is to move it out and run the server seperately

And finally, for it to actually be live I'd have to push that to the gh-pages branch (I'll do that eventually, don't want two PRs for one thing). That is because of my choice of repo layout though. Might not be optimal, but should never work on a live version (gh-pages) anyways.

SezerYardim commented 2 years ago

I ran the npm install command and installed dependencies. That package lock file changes every time when node_modules folder or package.json file changes. They say keep that file in your repo so i pushed it on purpose.

We should also let git to ignore the node_modules folder. Otherwise i am sure one day it will be pushed to the repo unintentionally

Seneral commented 2 years ago

Alright, that sounds good. Wasn't aware of best practices with node If you want to also push gitignore /w node_modules, which does make sense, I'd prefer that in a separate PR along with packacke-lock. But if you'd rather not, just add it to this PR. Am not rather specific about separating changes myself as you can see in most of my commits...

SezerYardim commented 2 years ago

I will do that in a seperate PR. Using yarn as a package manager might be a better idea. That way we dont have to deal with package-lock.json file

Seneral commented 2 years ago

Nah, not really needed here. I'll just merge for now