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

docs: updated readme files #1955

Merged
merged 7 commits into from
Apr 4, 2024
Merged

docs: updated readme files #1955

merged 7 commits into from
Apr 4, 2024

Conversation

shaharzil
Copy link
Contributor

@shaharzil shaharzil commented Feb 13, 2024

@shaharzil shaharzil requested a review from a team as a code owner February 13, 2024 15:38
@shaharzil shaharzil changed the title updated readme files docs: updated readme files Feb 13, 2024
Copy link
Member

@talkor talkor left a comment

Choose a reason for hiding this comment

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

Cool! Sorry for all these comments 😅

@@ -1,35 +1,12 @@
<!--
# Pull Request Template
Copy link
Member

Choose a reason for hiding this comment

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

I think it's a bit redundant and contributors will just leave it and it will be a bit weird

Copy link
Member

Choose a reason for hiding this comment

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

I still think it's a bit redundant

Comment on lines 3 to 4
Thank you for contributing!
Before we can review your submission, please fill the information below:
Copy link
Member

Choose a reason for hiding this comment

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

I think it can be commented out and also it's a bit funny to me that there's only a single checkbox. Maybe just reference the contribution guidelines in this commented-out comment?


#### Basic
Please describe the changes you're making. Include the motivation for these changes, any additional context, and the impact on the project. If your changes are related to any open issues, please link to them here.
Copy link
Member

Choose a reason for hiding this comment

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

This can also be commented out IMO

README.md Outdated
This functionality basically overrides the npm mapping between package name to its repo, and points it to where the package is located locally.

### Troubleshooting local development
This is a monorepo containing Vibe's maintained open-source packages.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This is a monorepo containing Vibe's maintained open-source packages.
This is a monorepo containing Vibe Design System open-source packages.

README.md Outdated

- If you are using NVM, make sure both packages are using the same version.
- Because we are using react hooks and having react as a peerDependency - if you want to develop locally and encounter issues with "invalid hook call" [see this github thread](https://github.com/facebook/react/issues/13991). The quick fix is in your webpack config file alias react to resolve the node_modules path
The packages are maintained using [lerna](https://github.com/lerna/lerna/tree/master/).
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's an important information, it won't matter for users, just for contributors

Copy link
Member

Choose a reason for hiding this comment

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

Also the same for the info below. I think it would be much more informative to include a list of the packages in the monorepo with links to their README, basic "getting started" guide and how to install the main package, a title of Contributing with a link to contribution guidelines

Comment on lines 84 to 86
## Contributing

We welcome every contributor, please read the [contribution guidelines](CONTRIBUTING.md) before submitting a PR
Copy link
Member

Choose a reason for hiding this comment

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

This should move the the main

Comment on lines +53 to +82
## Storybook

We are using storybook in order to develop the components independently of any consumer.
run this to build & run the storybook locally:

```bash
npm run storybook
```

the storybook will be served on `http://localhost:7007`

## Developing locally with your consumer application

When developing locally, we are using a npm functionality called npm link, this allows us to
work locally on our package and use it in a different project without publishing.
This functionality basically overrides the npm mapping between package name to its repo, and points it to where the package is located locally.

### Troubleshooting local development

- If you are using NVM, make sure both packages are using the same version.
- Because we are using react hooks and having react as a peerDependency - if you want to develop locally and encounter issues with "invalid hook call" [see this github thread](https://github.com/facebook/react/issues/13991). The quick fix is in your webpack config file alias react to resolve the node_modules path

Go to the project's directory and run:

```zsh
nvm use
npm unlink
npm link
npm start
```
Copy link
Member

Choose a reason for hiding this comment

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

This should be in the contribution guide


![image](https://user-images.githubusercontent.com/60314759/147566893-63c5209a-8b83-4f32-af61-8b4c350ec770.png)

[monday.com](https://www.monday.com) React components library - [Storybook](https://style.monday.com)
Copy link
Member

Choose a reason for hiding this comment

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

This should be in the root IMO

| --------- | --------- | --------- | --------- | --------- | --------- |
| last 4 versions| last 4 versions| 14+| last 2 versions| last 2 versions| last 2 versions |

## Installation
Copy link
Member

Choose a reason for hiding this comment

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

This should be in the root also IMO, event though it's related to core, since core is the core of the monorepo :)

import "monday-ui-react-core/tokens";
```

_If your project (or it's Storybook) is importing files differently - read more [here](DEPRECATED_IMPORTS.md)._
Copy link
Member

Choose a reason for hiding this comment

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

The link will be broken

@shaharzil shaharzil requested a review from talkor March 14, 2024 13:56
Copy link
Member

@talkor talkor left a comment

Choose a reason for hiding this comment

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

This is cool! We should be really precise here IMO as it's the face of the library, and should be as simple and easy-to-understand as possible so I wrote some more suggestions

@@ -1,35 +1,12 @@
<!--
# Pull Request Template
Copy link
Member

Choose a reason for hiding this comment

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

I still think it's a bit redundant

```
npm install monday-ui-react-core
```
Welcome to the Vibe Design System repository! This monorepo contains various open-source packages maintained by Vibe ([monday.com](https://www.monday.com)).
Copy link
Member

Choose a reason for hiding this comment

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

I don't think users care that it's a monorepo (beside contributors), we can call it a "project" and not a repository. Also I think it's better to rephrase is as "Vibe Design System, by monday.com), what do you think?

README.md Outdated

Components are imported from the library's root entry:
Vibe Design System offers a collection of packages designed to streamline your development process and enhance the user experience. Below is a list of our main packages:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Vibe Design System offers a collection of packages designed to streamline your development process and enhance the user experience. Below is a list of our main packages:
Vibe Design System is a collection of components and styles designed to streamline your development process and enhance the user experience. Below is a list of our main packages:

README.md Outdated
Comment on lines 11 to 12
1. [core](packages/core/README.md): [monday.com](https://www.monday.com) React components library - [Storybook](https://style.monday.com).
2. [style](packages/style/README.md): [monday.com](https://www.monday.com) styling foundations library.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should refer to Vibe here instead of monday.com, it seems repetitive, we mention in in the first paragraph

Copy link
Member

Choose a reason for hiding this comment

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

Also I don't think the internal structure is important for users at this point, it should be below "Getting Started"

README.md Outdated

We don't import fonts ourselves, we work best with the following fonts -
Poppins, Figtree and Roboto, we recommend adding the following `link` import to your application
Install the component library
Copy link
Member

Choose a reason for hiding this comment

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

This seem redundant

Comment on lines -63 to +28

## Developing locally with your consumer application
## Getting Started

When developing locally, we are using a npm functionality called npm link, this allows us to
work locally on our package and use it in a different project without publishing.
This functionality basically overrides the npm mapping between package name to its repo, and points it to where the package is located locally.
If you're new to Vibe Design System, we recommend starting with our main package:

### Troubleshooting local development
- [core](packages/core/README.md): This is the core of the Vibe Design System. Follow the installation guide and getting started instructions to integrate it into your project.
Copy link
Member

Choose a reason for hiding this comment

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

I think it's super confusing to have both sections (Installation and Getting Started), Installation can be a sub section in Getting Started with a referral to additional setup in the core README

## Themes

We support theming from the library to the component level using css variables - for more info on theming please read the [theme guidelines](THEME_README.md) file
We welcome every contributor, please read the [contribution guidelines](packages/core/CONTRIBUTING.md) before submitting a PR
Copy link
Member

Choose a reason for hiding this comment

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

We should also add a LICENSE section here

Copy link
Member

Choose a reason for hiding this comment

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

Worth adding a sentence about feature requests and bugs (report in issues/discussions)


## Themes

We support theming from the library to the component level using css variables - for more info on theming please read the [theme guidelines](./THEME_README.md) file
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
We support theming from the library to the component level using css variables - for more info on theming please read the [theme guidelines](./THEME_README.md) file
Theming is supported using CSS variables - for more info on theming please read the [theme guidelines](./THEME_README.md) file

Copy link
Member

Choose a reason for hiding this comment

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

Don't we also support ThemeProvider now?

Copy link
Member

@talkor talkor left a comment

Choose a reason for hiding this comment

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

Awesome

@shaharzil shaharzil merged commit e91376b into master Apr 4, 2024
10 checks passed
@shaharzil shaharzil deleted the fix-readmes branch April 4, 2024 10:15
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