Improve contributing guide #53
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xdrdak
reviewed
Nov 26, 2019
plopfile.js
Outdated
| plop.setGenerator('Add-on component', { | ||
| description: 'Create a new add-on component', | ||
| description: 'Create a new add-on/experimentall component', |
There was a problem hiding this comment.
Suggested change
| description: 'Create a new add-on/experimentall component', | |
| description: 'Create a new add-on/experimental component', |
xdrdak
reviewed
Nov 26, 2019
| describe('{{pascalCase name}}', () => { | ||
| it('should be tested', () => { | ||
| const { container } = customRender(<{{pascalCase name}}>children</{{pascalCase name}}>); | ||
| expect(container).toBeDefined(); |
There was a problem hiding this comment.
Funny enough, we could also fail the test to force folks to write something valid.
xdrdak
reviewed
Nov 26, 2019
| @@ -0,0 +1,3 @@ | |||
| import { {{pascalCase name}}, {{pascalCase name}}Props } from './{{pascalCase name}}'; | |||
There was a problem hiding this comment.
We might need to add isolated modules in our tsconfig. Should protect us from importing things that get stripped at build time.
What that means is the {{pascalCase name}}Props will not be valid. So it should be removed.
There was a problem hiding this comment.
This is the case with our latest tsconfig, should we update?
|
Deploy preview for lightspeed-flame ready! Built with commit 41570c9 |
xdrdak
reviewed
Nov 26, 2019
|
|
||
| const stories = storiesOf('{{pascalCase name}}', module); | ||
|
|
||
| stories.add('Story', () => ( |
There was a problem hiding this comment.
Can we swap this out to the new storybook api?
see https://storybook.js.org/docs/basics/writing-stories/#basic-story, which does not use the stories.add api
There was a problem hiding this comment.
For some reason couldn't get it to work. Let's scope this in another PR and update all stories to use the new Component Story Format (CSF).
Issue created: #54
.github/CONTRIBUTING.md
Outdated
|
|
||
| The main purpose of working on it open-source is to be transparent on what we bring to our customers, but also to be able to get valuable feedback and contribution from the community. We believe open-source is vital to software development as most of the libraries we rely on are built in the open space. | ||
|
|
||
| As our journey begins in the open-source world, we are accepting [bug reports](https://github.com/lightspeed/flame/issues/new?template=BUG_REPORT.md), [feature request](https://github.com/lightspeed/flame/issues/new?template=FEATURE_REQUEST.md) as well as pull requests for improvements, but no additions or breaking changes until we have a proper process to accept and treat those accordingly. |
There was a problem hiding this comment.
Suggested change
| As our journey begins in the open-source world, we are accepting [bug reports](https://github.com/lightspeed/flame/issues/new?template=BUG_REPORT.md), [feature request](https://github.com/lightspeed/flame/issues/new?template=FEATURE_REQUEST.md) as well as pull requests for improvements, but no additions or breaking changes until we have a proper process to accept and treat those accordingly. | |
| As our journey begins in the open-source world, we are accepting [bug reports](https://github.com/lightspeed/flame/issues/new?template=BUG_REPORT.md), [feature requests](https://github.com/lightspeed/flame/issues/new?template=FEATURE_REQUEST.md) and pull requests for improvements. We are not accepting additions or breaking changes until we have a proper process to triage and approve these changes accordingly. |
.github/CONTRIBUTING.md
Outdated
|
|
||
| As our journey begins in the open-source world, we are accepting [bug reports](https://github.com/lightspeed/flame/issues/new?template=BUG_REPORT.md), [feature request](https://github.com/lightspeed/flame/issues/new?template=FEATURE_REQUEST.md) as well as pull requests for improvements, but no additions or breaking changes until we have a proper process to accept and treat those accordingly. | ||
|
|
||
| We still encourage anyone to fork and play around Flame for personal usage. Read our [LICENSE](../LICENSE) in case of uncertainty on what we allow in this regard. |
There was a problem hiding this comment.
The second sentence is a little vague, but I guess the license only pertains to users who would like to use Flame commercially. If I'm correct, consider the following suggestion:
Suggested change
| We still encourage anyone to fork and play around Flame for personal usage. Read our [LICENSE](../LICENSE) in case of uncertainty on what we allow in this regard. | |
| We still encourage anyone to fork and play around Flame for personal usage. Read our [LICENSE](../LICENSE) in the case of commercial use. |
.github/CONTRIBUTING.md
Outdated
|
|
||
| #### Mainline component | ||
|
|
||
| Mainline components will be created under `packages/flame/src/{ComponentName}`, where all other components are located. This will create the base component structure ready for development, including README for docs, story for Storybook development and visual regression test, tests and TypeScript component file. |
There was a problem hiding this comment.
Usually, when we create a comma separated list, we aim to have three separate items that are fairly similar in length. The flow helps with readability. Consider the following suggestion:
Suggested change
| Mainline components will be created under `packages/flame/src/{ComponentName}`, where all other components are located. This will create the base component structure ready for development, including README for docs, story for Storybook development and visual regression test, tests and TypeScript component file. | |
| Mainline components will be created under `packages/flame/src/{ComponentName}`, where all other components are located. The base component structure includes a storybook file, a test file, and a component file. |
|
|
||
| #### Add-on component | ||
|
|
||
| Add-on component will be created under the `packages/flame-{component-name}`. It should be used for add-on/experimental components that are not ready to go into the [mainline package](https://github.com/lightspeed/flame/tree/master/packages/flame) or are considered as "extras" and don't fit our component library. |
There was a problem hiding this comment.
Restructured to explain what is defined as an "add-on" component:
Suggested change
| Add-on component will be created under the `packages/flame-{component-name}`. It should be used for add-on/experimental components that are not ready to go into the [mainline package](https://github.com/lightspeed/flame/tree/master/packages/flame) or are considered as "extras" and don't fit our component library. | |
| Add-on components will be created under the `packages/flame-{component-name}`. These components are either experimental in implementation, or compositions that do not fit our essential component library. |
|
|
||
| Add-on component will be created under the `packages/flame-{component-name}`. It should be used for add-on/experimental components that are not ready to go into the [mainline package](https://github.com/lightspeed/flame/tree/master/packages/flame) or are considered as "extras" and don't fit our component library. | ||
|
|
||
| Add-on components are a completely separate package with their own version and have `@lightspeed/flame` and all its required dependencies as `peerDependencies`. They include the same base component structure as a mainline component, but also have a `tsconfig` to output types as well as their own LICENSE and CHANGELOG files. |
There was a problem hiding this comment.
I don't think we need to be too granular in the component structure (re: CHANGELOG and LICENSE) if you word this section as follows:
Suggested change
| Add-on components are a completely separate package with their own version and have `@lightspeed/flame` and all its required dependencies as `peerDependencies`. They include the same base component structure as a mainline component, but also have a `tsconfig` to output types as well as their own LICENSE and CHANGELOG files. | |
| Add-on components are a completely separate package which are versioned independently, licensed independently, and have `@lightspeed/flame` and all its required dependencies as `peerDependencies`. They include the same base component structure as a mainline component. |
Independent versioning and licensing implies that the package has its own LICENSE and CHANGELOG.
immasandwich
suggested changes
Nov 26, 2019
|
Thanks for the review @xdrdak and @kshahani. Ready for another check! |
immasandwich
approved these changes
Nov 26, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Our contributing guide needed a bit of ❤️, especially for external contributors. I've also added a
yarn generatecommand for mainline components.How to test?
See changed files for the contributing guide updates.
For the new generate script:
yarn generateyarn devChecklist