Improvement/1st improvement design system #253

Cuervino · 2021-03-02T14:25:15Z

Component: storybook, design system, tests

Description:

A big PR but not that difficult to review. On the 6 commits only 2 are really important :

storybook: update storybook settings
mdx: add first design system bricks

You also might want to review it by launching the storybook.
You will need to npm install before npm run storybook because we added some changes to handle Windows environnement.

The rests are cosmetics changes and fix of tests.

bert-e · 2021-03-02T14:25:17Z

Hello cuervino,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

bert-e · 2021-03-02T14:25:20Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

Co-authored-by: Cuervino <[email protected]>

- Stories are now require to finish by .stories.js - Prepare the sorting of the stories Co-authored-by: Cuervino <[email protected]>

We add ".stories" ending at the end of stories files because with MDX, we might want to render custom components that are not stories. Currently, anything that is exported in a file in /stories/* will be displayed in storybook. We want a way to discriminate files in the stories/ folder that will not be displayed by storybook. So, I change the `main.js` (previous commit) in order to filtrate this kind of files. Now you will be able to create custom component to render in your MDX files by creating files that do not end with `.stories.js`.

1) New hierarchy 2) New pages that are not stories 3) Icon list 4) In mdx file, better to import id stories (defined in the related js) rather than creating one in the mdx 5) WIP for chart guideline Co-authored-by: Cuervino <[email protected]>

https://github.com/storybookjs/storybook/tree/master/addons/storyshots/storyshots-core#multisnapshotwithoptionsoptions Please note that I haven't include the integrityOptions because I don't really know what it does and it break the test for no special reason IMO. If you find out, what's wrong please create an issue to fix it.

nicolas2bert · 2021-03-03T14:10:38Z

.storybook/preview.js

+        ["Color", "Icons"],
+        "Components",
+        [
+          "Navigation",


Should it be ordered alphabetically for better readability?

For headers on level 1 (STYLE, COMPONENT), the order should be predefined.
For the Intro and Style section, the order of levels 2 should be predefined.
For the library itself (inside COMPONENTS), it could be alphabetical order, however it makes sense to put Navigation at the beginning, to set Input and Selector next to each other, and also Chart and Progress & loading aside, as they are rather visual components.
At the end/bottom, it could be miscellaneous components.

Thank you for this explanation.

nicolas2bert · 2021-03-03T14:33:42Z

package.json

@@ -8,7 +8,7 @@
  "mainSrc": "src/lib/index.js",
  "scripts": {
    "analyze": "source-map-explorer 'dist/*.js'",
-    "build": "rimraf dist && NODE_ENV=production babel src/lib --out-dir dist --copy-files && rimraf dist/**/*.test.js && rimraf dist/**/__snapshots__",
+    "build": "rimraf dist && cross-env NODE_ENV=production babel src/lib --out-dir dist --copy-files && rimraf dist/**/*.test.js && rimraf dist/**/__snapshots__",


What about using envname option to avoid having another dependency? https://babeljs.io/docs/en/options#envname

We can, but I don't think it's an issue. cross-env is a widely use stable dependency to handle this use case.
And because it's only use during the build, it doesn't have an impact on the size on the build itself.

A big reason why you use a library in the first place is to get the benefits of the code without having to worry about all the details. The drawback is that you’ve completely tied your code to these dependencies that you don’t own and don’t control. Instead, you could have used an existing babel option.

nicolas2bert · 2021-03-03T14:42:31Z

.prettierrc

@@ -1,4 +1,5 @@
 {
  "tabWidth": 2,
-  "trailingComma": "all"


Why do we need two different linter tools? ESLint should handle code formatting concerns.

On metalk8s and xcore. We use ESLint for code quality rules and prettier for formatting rules.
ESLint can indeed do both but I (subjective opinion) prefer prettier.

I have never user Prettier.io and would love to have your opinion on "what is the advantage of using Prettier for formatting concerns over Eslint?". We could have a side discussion on this topic if you want.

nicolas2bert · 2021-03-03T15:23:26Z

stories/sparkline.stories.js

@@ -29,7 +29,7 @@ const id_multi = "multiVis";
 const lineConfig = { interpolate: "monotone" };

 export default {
-  title: "SparkLine",
+  title: "Components/Chart/SparkLine",


For maintainability purposes, we could assign variables for each topic/subtopic.
These values should be aligned with the .storybook/preview.js ones.

Yes, agree, and it would be easier to change/to maintain.
However, as it's a limited number of items for the moment, and as it might not save time to define variables, we can keep it like that for this version.

nicolas2bert · 2021-03-03T15:27:41Z

stories/icons.stories.mdx

+    <td></td>
+    <td></td>
+  </tr>
+  <tr>


Not sure to understand this last row.

Agree, not clear: it's not a real icon.
In the case of a cell with no value in a table, we use a simple "hyphen" (-) from the font, for still displaying there is no value, without having too much visual noise.
As it's still useful to know about this specific use case, this info could be added in the "Table" component/guideline.

stories/guideline/button-guideline.stories.mdx

stories/guideline/chart-guideline.stories.mdx

stories/guideline/mdxExampleComponents.js

Co-authored-by: JBWatenbergScality <[email protected]>

JBWatenbergScality

Looks good to me 👍 , I purposed a set of suggestions to unify the case of the list items in each documentation but I let you see if it makes sense.

stories/Introduction.stories.mdx

stories/guideline/button-guideline.stories.mdx

stories/guideline/chart-guideline.stories.mdx

stories/guideline/button-guideline.stories.mdx

Co-authored-by: JBWatenbergScality <[email protected]>

Cuervino · 2021-03-05T13:08:16Z

/approve

bert-e · 2021-03-05T13:08:22Z

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

✔️ development/1.0

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

Any commit you add on the source branch will trigger a new cycle after the
current queue is merged.
Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

bert-e · 2021-03-05T13:08:27Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/1.0

Please check the status of the associated issue None.

Goodbye cuervino.

MonPote force-pushed the improvement/1st-improvement-design-system branch 5 times, most recently from 638b2d5 to 47093d5 Compare March 3, 2021 13:00

package: add cross-env and update tests settings

219accf

Co-authored-by: Cuervino <[email protected]>

MonPote force-pushed the improvement/1st-improvement-design-system branch from 47093d5 to b7d0339 Compare March 3, 2021 13:06

MonPote and others added 5 commits March 3, 2021 14:08

config: update gitignore and prettierrc

23665f0

Co-authored-by: Cuervino <[email protected]>

storybook: update storybook settings

801b2fb

- Stories are now require to finish by .stories.js - Prepare the sorting of the stories Co-authored-by: Cuervino <[email protected]>

mdx: add first design system bricks

67cee5f

1) New hierarchy 2) New pages that are not stories 3) Icon list 4) In mdx file, better to import id stories (defined in the related js) rather than creating one in the mdx 5) WIP for chart guideline Co-authored-by: Cuervino <[email protected]>

MonPote force-pushed the improvement/1st-improvement-design-system branch from b7d0339 to e6e8c5f Compare March 3, 2021 13:09

MonPote marked this pull request as ready for review March 3, 2021 13:33

MonPote requested review from ChengYanJin, JBWatenbergScality and nicolas2bert March 3, 2021 13:36

nicolas2bert reviewed Mar 3, 2021

View reviewed changes

JBWatenbergScality reviewed Mar 3, 2021

View reviewed changes

MonPote and others added 2 commits March 4, 2021 11:37

Update stories/guideline/chart-guideline.stories.mdx

572c3b0

Co-authored-by: JBWatenbergScality <[email protected]>

changes for the comments

5876451

JBWatenbergScality approved these changes Mar 5, 2021

View reviewed changes

Cuervino and others added 6 commits March 5, 2021 13:58

Update stories/Introduction.stories.mdx

85cee92

Co-authored-by: JBWatenbergScality <[email protected]>

Update stories/guideline/chart-guideline.stories.mdx

62ed29f

Co-authored-by: JBWatenbergScality <[email protected]>

Update stories/guideline/button-guideline.stories.mdx

d020ac9

Co-authored-by: JBWatenbergScality <[email protected]>

Update stories/guideline/button-guideline.stories.mdx

9d8f0e6

Co-authored-by: JBWatenbergScality <[email protected]>

Update stories/guideline/button-guideline.stories.mdx

4a9c09d

Co-authored-by: JBWatenbergScality <[email protected]>

Update stories/guideline/chart-guideline.stories.mdx

7ffbfd2

Co-authored-by: JBWatenbergScality <[email protected]>

MonPote approved these changes Mar 5, 2021

View reviewed changes

bert-e merged commit 7ffbfd2 into development/1.0 Mar 5, 2021

Cuervino deleted the improvement/1st-improvement-design-system branch March 5, 2021 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvement/1st improvement design system #253

Improvement/1st improvement design system #253

Cuervino commented Mar 2, 2021 •

edited by MonPote

Loading

bert-e commented Mar 2, 2021

bert-e commented Mar 2, 2021

nicolas2bert Mar 3, 2021

Cuervino Mar 3, 2021

nicolas2bert Mar 5, 2021

nicolas2bert Mar 3, 2021

MonPote Mar 4, 2021

nicolas2bert Mar 5, 2021

nicolas2bert Mar 3, 2021

MonPote Mar 4, 2021

nicolas2bert Mar 5, 2021

nicolas2bert Mar 3, 2021

Cuervino Mar 3, 2021 •

edited

Loading

nicolas2bert Mar 3, 2021

Cuervino Mar 3, 2021

JBWatenbergScality left a comment

Cuervino commented Mar 5, 2021

bert-e commented Mar 5, 2021

bert-e commented Mar 5, 2021

Improvement/1st improvement design system #253

Improvement/1st improvement design system #253

Conversation

Cuervino commented Mar 2, 2021 • edited by MonPote Loading

bert-e commented Mar 2, 2021

Hello cuervino,

bert-e commented Mar 2, 2021

Waiting for approval

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cuervino Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JBWatenbergScality left a comment

Choose a reason for hiding this comment

Cuervino commented Mar 5, 2021

bert-e commented Mar 5, 2021

In the queue

bert-e commented Mar 5, 2021

Cuervino commented Mar 2, 2021 •

edited by MonPote

Loading

Cuervino Mar 3, 2021 •

edited

Loading