search

flexdinesh / react-socks

🎉 React library to render components only on specific viewports 🔥
MIT License
426 stars 29 forks source link

implement setDefaultWidth method on breakpoint-util #40

Closed hems closed 3 years ago

flexdinesh commented 4 years ago

Thanks @hems. This seems like an good option. Why did you remove the dist entry from .gitignore? We don't want to push build files to the repo.

hems commented 4 years ago

Thanks @hems. This seems like an good option. Why did you remove the dist entry from .gitignore? We don't want to push build files to the repo.

I understand where you're coming from, but when I forked into my repo I had to change my package.json to use my github address instead of your official version on NPM and since the dist folder was not on the repo my application was not able to import the files, basically breaking my application.

The only way i would go around that would be to publish my fork to npmjs which also would be a lot of unnecessary work.

TLDR: The only way i was able to use my fork/commit/branch version of the library from github as dependency on package.json we need to publish the dist folder.

Do you have a better solution for this issue?

That's how my package.json looks like now:

  "dependencies": {
...
    "react-socks": "hems/react-socks#1798789784827cfb577b79a3b5c4ad86c3c05c7d",
...
  }

from my personal experience with npm and github sooner or later I had issues if publishing different files on github and npmjs, so i rather have the dist folder published, they're just text files, it's not like it will take a lot of space..

let me know your thoughts on that!

twhite96 commented 4 years ago

Is this going to be merged @flexdinesh?

flexdinesh commented 4 years ago

Sorry for the delay. OSS is sometimes overwhelming. I should've gotten to this sooner nevertheless. I'm down to merge once @hems reverts the build dir entry .gitignore. If not I can do that as well.

flexdinesh commented 4 years ago

@hems For your fork it works well. If I had a fork I'd follow the same approach you took. I'd point directly to my github fork and include build files in the repo instead of publishing to npm. But we don't want to have the build dir in this repo 'cos they don't serve any purpose.

Would you be able to revert the entry from .gitignore for this PR? I can do that on your behalf as well.

hems commented 4 years ago

But we don't want to have the build dir in this repo 'cos they don't serve any purpose.

they do serve the purpose of being able to point to any specific commit on github, IMHO that's an important purpose enough.

also means that if people fork and submit PRs back to the original repo they won't have to be thinking about not sending .gitignore to the main branch.

Would you be able to revert the entry from .gitignore for this PR? I can do that on your behalf as well.

I won't be able to do as I'm not working anymore with react-socks i just kept the repo on my profile in order to don't break this PR.

nawazeverlane commented 3 years ago

@flexdinesh Can I take over this PR? This seems to block the server-side rendering part 🙏🏽

flexdinesh commented 3 years ago

Yes please mate. I could use the help. @nawazeverlane

flexdinesh commented 3 years ago

@hems Thanks for your time and input on this PR and the issue #30 mate. It was very helpful. I was a bit busy with other projects but with @nawazeverlane's help we have now addressed the issue with #44. I've added you the contributors list as well since you work laid all the groundwork to get this going.

If you want to talk about .gitignore, please create a separate issue and we'll discuss it there. Thanks again.

hems commented 3 years ago

@hems Thanks for your time and input on this PR and the issue #30 mate. It was very helpful. I was a bit busy with other projects but with @nawazeverlane's help we have now addressed the issue with #44. I've added you the contributors list as well since you work laid all the groundwork to get this going.

If you want to talk about .gitignore, please create a separate issue and we'll discuss it there. Thanks again.

thanks a lot for adding me to the list of contributors i really appreciate.

regarding .gitignore i don't have a reason to keep talking about it.

As i said but ignoring the build files you're actually creating a problem for people like me who fork the project or submit a PR and need to run a specific commit version before the project is published on npm.

Also, on my personal opinion there is no reason for not having the build files on the repo, so for me the discussion is done!

Anyway, it's your project, so it's really up to you.

thanks a lot for the work you have put in this library, it's pretty fun to use it!

☮️ ✌️