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

Adding useClickOutsideCallback hook from 2u #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

refactorized
Copy link

🚧 progress on adding 2u useOutsideClickCallback

🚧 (@hzcore/hook-click-outside-callback) more docz, spiffy stylez

pretty much done with this one, two snazzy examples and some prose, but need a little more work
documenting proper typescript usage

🚧 (@hzcore/hook-click-outside-callback) polish

wip fighting commitizen

🚧 (@hzcore/hook-click-outside-callback) more docz, spiffy stylez

pretty much done with this one, two snazzy examples and some prose, but need a little more work
documenting proper typescript usage

🚧 (@hzcore/hook-click-outside-callback) polish

wip fighting commitizen
@refactorized refactorized requested a review from lettertwo March 10, 2020 14:05
@@ -0,0 +1,23 @@
{
"name": "@hzcore/hook-click-outside-callback",
"version": "0.0.1",
Copy link
Contributor

@lettertwo lettertwo Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a short description from the README could be added here in a "description" field.

],
"dependencies": {
"mini-svg-data-uri": "^1.1.3"
}
Copy link
Contributor

@lettertwo lettertwo Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add react to "peerDependencies", and "@babel/runtime" to dependencies, like:

Suggested change
}
},
"peerDependencies": {
"react": "^16.8.0"
},
"dependencies": {
"@babel/runtime": "^7.4.4"
}

Rationale

React as a peer dependency because we depend on react, but we don't want to implicitly choose the version of React for the user's application by forcing install (nor do we want to risk introducing duplicate versions of React to the dependency tree!)

@babel/runtime as a dependency because we use @babel/plugin-transform-runtime when building hzcore packages, which will introduce the runtime as a dependency as part of the transform.

@@ -0,0 +1,23 @@
{
"name": "@hzcore/hook-click-outside-callback",
"version": "0.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing i've been doing to prep new packages for their first publish is to set the version to a major alpha, like:

Suggested change
"version": "0.0.1",
"version": "1.0.0-alpha.0",

This results in lerna rolling us up to 1.0.0 automatically on first publish.

"module": "es/index.js",
"typings": "src/index.tsx",
"license": "MIT",
"private": true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to make the package 'public', even though we'll be publishing to a private registry:

Suggested change
"private": true,

"!**/examples",
"!**/__test*"
],
"dependencies": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks likemini-svg-data-uri is only used in the example, which I think means it should be in "devDependencies":

Suggested change
"dependencies": {
"devDependencies": {

import Styles from './ReadmeStyles.tsx';

# useClickOutsideCallback

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like to add a little description sentence here, like A react hook for <insert reason to use this package here>


test('clickOutsideCallback is implemented', () => {
expect(() => clickOutsideCallback()).not.toThrow();
throw new Error('implement clickOutsideCallback and write some tests!');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll have to make this test pass before we publish. Nice to have: some more tests!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to merge without tests, unless we have a pressing need for this component.

Copy link
Contributor

@lettertwo lettertwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like .DS_Store files have infected the branch. We should strip them from the repo (optionally, from the history entirely) and add them to the .gitignore:

https://stackoverflow.com/questions/107701/how-can-i-remove-ds-store-files-from-a-git-repository

I'd also recommend adding them to your global .gitignore config, since these files are never useful on any other system.

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

Successfully merging this pull request may close these issues.

2 participants