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

Update README file #147

Merged
merged 3 commits into from
Jan 12, 2021
Merged

Update README file #147

merged 3 commits into from
Jan 12, 2021

Conversation

liatgo
Copy link
Contributor

@liatgo liatgo commented Jan 7, 2021

📦 Published PR as canary version: Canary Versions

✨ Test out this PR locally via:

npm install @kenshooui/react-tree@0.0.19-canary.147.400.0
npm install @kenshooui/material-tree@0.0.2-canary.147.400.0
# or 
yarn add @kenshooui/react-tree@0.0.19-canary.147.400.0
yarn add @kenshooui/material-tree@0.0.2-canary.147.400.0

@liatgo liatgo requested a review from liorheber January 7, 2021 18:24
@@ -25,6 +25,7 @@ const Tree = props => {
onSelect,
width,
height,
styles,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liorheber In the previous PRs I removed "styles" and "classname" props bacuse I thought They are not in use.
Noe when writing the README I understood the "styles" is needed for customized styles.
What about calssname? Do we need this property?

Copy link
Contributor

Choose a reason for hiding this comment

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

Which classNames? Can you link the change?
You would usually need the className prop to customize using almost any styling library - css modules, styled-components etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

They are for customization, however, I'm not sure it was fully implemented.
I suggest returning it and we can have a look at it at a later task when we decide to support that type of customization.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Comment on lines +20 to +26
npm install @kenshooui/material-tree --save

npm install @kenshooui/react-tree --save

npm install @material-ui/core --save

npm install @material-ui/icons --save
Copy link
Contributor

Choose a reason for hiding this comment

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

You can do this, and the yarn command in a single line -
npm install --save @material-ui/core @material-ui/icons @kenshooui/react-tree @kenshooui/material-tree
yarn add @material-ui/core @material-ui/icons @kenshooui/react-tree @kenshooui/material-tree

Also worth mentioning that the material dependencies are only if they don't exist already in the app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liorheber isn't this enough: "In addition, dependencies to material-ui icons and core components are needed if they don't exist in your project." ?
Do you want me to rephrase it?

@liorheber liorheber added the skip-release Preserve the current version when merged label Jan 12, 2021
@liorheber
Copy link
Contributor

Merging this, please fix my comments in another PR.
When you issue the PR for it, there's a label section in Github, use the skip-release label so we can avoid releasing a new version to npm for the documentation.

@liorheber liorheber removed the skip-release Preserve the current version when merged label Jan 12, 2021
@liorheber liorheber merged commit a8bb716 into master Jan 12, 2021
@liorheber liorheber deleted the update-readme-file branch January 12, 2021 02:06
@liorheber
Copy link
Contributor

You also need to place the API documentation of the core component in the core package.
The root should be the link to the two.

@liatgo
Copy link
Contributor Author

liatgo commented Jan 12, 2021

Merging this, please fix my comments in another PR.
When you issue the PR for it, there's a label section in Github, use the skip-release label so we can avoid releasing a new version to npm for the documentation.

@liorheber This PR included few changes in the components as well. Do we want to avoid releasing in this case?

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