-
Notifications
You must be signed in to change notification settings - Fork 61
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
Feat/guidelines ds #692
Feat/guidelines ds #692
Conversation
samuel-gomez-axa
commented
Oct 6, 2020
- some icones size fixed
- add guidelines UX
- add documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job Samuel ! Awesome
Wow, that's a huge PR and a lot of work! Thanks very much! Just noticed that the following lines are still in the package.json, but I think they are useless (l. 18 & 19): "css": "npm -C ./storybook/styles run css",
"css:build": "npm -C ./storybook/styles run css:build", Maybe you can remove them as well? |
Sure, you're right @gcruchon , I propose to do this in another PR because this is not the subject of this PR, ok ? |
# Contributing to @axa-fr/react-toolkit | ||
|
||
First, ensure you have Node.js < 12 and the [latest `npm`](https://docs.npmjs.com/). | ||
## Installation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why removed the 1st level block? I think it should be kept (MDLint fails on this). Plus, it gives the title of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i just wanted to delete the node version requirement. i didn't want to delete the title. i'll fixed it, thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in last commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
CONTRIBUTING.md
Outdated
cd examples/demo | ||
npm i | ||
``` | ||
|
||
```sh | ||
cd storybook/design-system | ||
npm i |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arnaudforaison changed it to npm ci
on #691, for installing on a clean slate. Should we keep "i" or use "ci"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "ci" it's more appropriated
CONTRIBUTING.md
Outdated
``` | ||
|
||
to regenerate css | ||
After all this, if your development affect css, you can run `npx gulp all` to regenerate css |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gulp has been removed. Let's change this to npm run style
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in last commit
No problem. Let's add an issue, then? I can contribute :) |
Kudos, SonarCloud Quality Gate passed! 0 Bugs |
Hello, Looks like there are some broken dependencies in non committed pug files. As a consequence, the design-system package cannot build. E.g. : /storybook/design-system/src/components/form/mixins/mixin-form-field-card.pug, line 7: include ../../../pages/style/icons/templates/ok.pug This template has moved to WIth a search in files on BTW, the french |