Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can I PR new functions? #81

Closed
PabloLION opened this issue Jan 11, 2022 · 13 comments · Fixed by #89
Closed

Can I PR new functions? #81

PabloLION opened this issue Jan 11, 2022 · 13 comments · Fixed by #89

Comments

@PabloLION
Copy link
Contributor

PabloLION commented Jan 11, 2022

Hi team.
I made a ternary Darkmode (based on this repo) like "dark" | "system" | "light" in a project and I want to use it in another project.
The best way to share it is to upload it on NPM and GitHub. And I think putting it here might be helpful, as I see you have " Add new hooks" in your roadmap. Questions are:

  1. Is this too trivial to add?
  2. Can I make a PR on this repo?
  3. Do you have a Code of Conduct?

Thanks.

@juliencrn
Copy link
Owner

Hey @PabloLION,

That's a good idea! There is already a hook about dark theme, but your feature is a bit different, it's really welcome!!
We haven't Code of Conduct yet, but we have some tools to drive style like Eslint, Plop, Prettier, Typescript.
So, you can fork and install locally, then to create a brand-new hook, we have a CLI command ready to use npm run plop, it generates all the necessary boilerplate (hook itself, tests, markdown description, demo, link to the index files).
Once your hook is ready and passing the tests, you can submit your PR.
(Additionally, It could be cool to add a mention "related hook" in both dark theme hooks, but I can do it later if you want).

I'm looking forward to reviewing it!

@PabloLION
Copy link
Contributor Author

I know about that hook, and I tried it. But after I made it I find Mac OS is using this ternary dark mode. (system-> automatic) And if we use what system uses, it would be also automatic. This time apple acted faster than me XD.

The plop is useful. I'll try to run it before I start, then copy paste my code and tinker a little. (Maybe you can write a short contribution chapter?)

And do you have any idea on naming? I'm going to start working on it now.

@PabloLION
Copy link
Contributor Author

Hi again @juliencrn. I need some guide on the .demo.tsx. I saw the third line of site/src/hooks-doc/useDarkMode/useDarkMode.demo.tsx, shown as below, is reporting Cannot find module 'usehooks-ts' or its corresponding type declarations.ts(2307). What should I do to test the demo?

import { useDarkMode } from 'usehooks-ts'

@egehandulger
Copy link
Contributor

@PabloLION I am not sure whether it is the correct way to go but since there is a symlink i think reloads are not recognised. Assuming you have VSCode; after you make a change and compile, while a TypeScript file is open hit cmd + shift + p and run TypeScript: Restart TS server. Wait until it finishes restarting and you should see the updates.

@PabloLION
Copy link
Contributor Author

Thanks for the reply, @egehandulger. The compile you mean "npm start" or "tsc"? I checked the package.json and I didn't see compile command.

@egehandulger
Copy link
Contributor

@PabloLION Yes, I mean npm start as it compiles the files you change. Sorry for the confusion.

@juliencrn
Copy link
Owner

Hi, yes @PabloLION, you're right, I will add a contribution guide, it will be easier for someone to contribute.

And yes @egehandulger exact; the npm start is a dev command, but you must compile the lib before playing with the frontend. The live reload works:

  • in the lib only ✅
  • on the website only ✅
  • but not in both at the same time 🟥
    It must be improved too.

The compile script exists but named build.

To play with the demo locally, it is a little tricky too for now, you can import it in the pages/test-page.tsx and do not git commit it.

Thanks to you guys, I'm now convicted that I really need to improve the developer experience, by fixing the points told below. I will do it sooner!

You maybe could name it useSystemTheme, what do you think about it ?
I'm thinking about your hook could replace the current useDarkTheme in the next release. It seems to me better.

Thank you 🤗

@PabloLION
Copy link
Contributor Author

PabloLION commented Jan 12, 2022

Hi @juliencrn. Than you for the details. I was also thinking about to summarize these things I learned there and make another PR on the README for the "contribution" chapter. But obviously you will do it better than me.

As for the name, "useDarkMode" explains clearly what it means. "useSystemTheme" is not as good as "dark mode" since windows has its own "theme" and Mac has an "accent color", yet we still related, better than how I called it("usePaletteMode"). On the other hand, "useTernaryDarkMode" is not so elegant but more explicit, I guess everyone sees the name knows its meaning.

Also, I don't think the old "useDarkMode" need to be deprecated. Some people just prefer everything easier.

PS I forgot that I have this deployed on https://SnippetHub.dev , an unfinished project. You can see the effect there.

@juliencrn
Copy link
Owner

useTernaryDarkMode seems to be well :)

If you want to create/improve the "contribution" chapter, feel free, it's really welcome. You have been just blocked by its absence, so you know well what is missing, what was blocking you.

@juliencrn
Copy link
Owner

Hello @PabloLION, I've updated the readme.md about how to contribute. What do think about, it's better? Is missing something?

@PabloLION
Copy link
Contributor Author

PabloLION commented Jan 18, 2022

@juliencrn Sorry I was busy lately. Today hopefully I'll finish this PR. I pulled the new master and did a rebase on my branch.

how to contribute

  1. It seems you have two typos npm run dev:lib, which should bestart:lib. There's no dev
  2. problem while testing the site with npm run start:site: the website is not reacting to the change of .demo.tsx. I tried with npm start mentioned by @egehandulger and it worked.
  3. About the

in the lib only ✅
on the website only ✅
but not in both at the same time 🟥

While using npm run start:lib, the localhost:8000 is not running, and I don't know how to test the demo as you wrote "It's displayed as an interactive sandbox in the hook page during the dev." I think we need to be clearer on this.
4. About npm run test. Since we have .husky maybe use some git hook way to do it (I don't know how).

About this PR

  1. I can't test the demo, as mentioned in last chapter(3.)
  2. Tested on codeSandbox changing some import https://codesandbox.io/s/usehooks-ts-react-component-template-forked-lprwq?file=/src/App.tsx

@juliencrn
Copy link
Owner

Hi @PabloLION, thanks for your feedback (and for the PR 🎉 )

It seems you have two typos npm run dev:lib, which should bestart:lib. There's no dev

Good catch, it's true on your version of master, but I've renamed it to be more explicit.

with npm run start:site: the website is not reacting to the change of .demo.tsx.

Hm weird, it works well on my machine. And, the .demo.tsx are in the site/src folder and should be reactive.

While using npm run start:lib, the localhost:8000 is not running

True, it was the case before, it was only watching files changes to re-build the lib. I've removed it, now there is only npm run dev:lib to launch jest --watch. However, it doesn't run localhost:8000, the site is another package.

I don't know how to test the demo as you wrote "It's displayed as an interactive sandbox in the hook page during the dev."

Indeed, I think we need to be clearer on this.
I take note, thanks. I'll update this.

  1. About npm run test. Since we have .husky maybe use some git hook way to do it (I don't know how).

If there is no bug, it should work. Husky is installed when you run npm install. Then there is a husky script (.husky/pre-push - executed by husky) that's executing an npm run test on pre-push. I think you can verify with a cli git push reading the logs.

@PabloLION
Copy link
Contributor Author

Good catch, it's true on your version of master, but I've renamed it to be more explicit.

I forked this repo and I didn't fetch on my forked repo.

Hm weird, it works well on my machine. And, the .demo.tsx are in the site/src folder and should be reactive.

I'll test with the latest commit again.

Indeed, I think we need to be clearer on this.

I just saw that it's under /test-page

Husky

That means we don't need to call out to run npm run test manually in the "How to contribute" Chapter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants