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

feat(storybook): upgrade story book to support mdx files #210

Merged
merged 24 commits into from
Aug 12, 2024

Conversation

manojava-gk
Copy link
Contributor

@manojava-gk manojava-gk commented Jun 13, 2024

Description

update storybook versions and support mdx files
add new component specific mdx files
table component content also added

Why

move design guidelines to storybook from assets

Issue

#115

#223

Checklist

Please delete options that are not relevant.

  • I have followed the contributing guidelines
  • I have performed IP checks for added or updated 3rd party libraries
  • I have created and linked IP issues or requested their creation by a committer
  • I have performed a self-review of my own code
  • I have successfully tested my changes locally
  • I have added tests that prove my changes work
  • I have checked that new and existing tests pass locally with my changes
  • I have commented my code, particularly in hard-to-understand areas

Copy link

sonarcloud bot commented Jun 13, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@manojava-gk
Copy link
Contributor Author

@evegufy Can you pls check dependency issue here?

@evegufy
Copy link
Contributor

evegufy commented Jun 18, 2024

I checked an the issue in the PR checks is specific to this PR and connected to some storybook package upgrade, not a general issue.
@oyo is checking it out further

@oyo
Copy link
Contributor

oyo commented Jun 18, 2024

I checked an the issue in the PR checks is specific to this PR and connected to some storybook package upgrade, not a general issue. @oyo is checking it out further

After some investigation Storybook from v8.1.0 onwards requires prettier to be added as a peer dependency. So I moved it from devDependencies to dependencies and the prettier check is now running.

<br />
<br />

## NOTICE
Copy link
Contributor

@evegufy evegufy Jun 19, 2024

Choose a reason for hiding this comment

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

@manojava-gk coming back to our discussion from yesterday, as you were asking me if an md file necessarily needs this notice footer (with this example https://github.com/catenax-ng/tx-portal-assets/blob/main/docs/user/Style-Guide/button%20load%20more.md), I was answering with yes because markdown files are considered documentation.
I realised after our discussion that you're moving the contents into mdx files, which can be considered source code, so you should change to file header like for any other source file and remove the notice footer.

CHANGELOG.md Outdated
@@ -1,5 +1,9 @@
# Changelog

## 3.0.23

- Support mdx file
Copy link
Contributor

@evegufy evegufy Jun 19, 2024

Choose a reason for hiding this comment

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

I don't think this is an accurate description of what's happening, something like "Integrate style-guide documentation into shared components and enable MDX support" would be more appropriate

Copy link

Choose a reason for hiding this comment

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

agree and where do we have the storybook version upgrade?

Copy link
Contributor

Choose a reason for hiding this comment

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

please also raise the minor version to 3.1.0, as this merits a minor change and not just a patch

@@ -0,0 +1,81 @@
{/**\*\*\*\***\*\***\*\*\*\***\*\*\*\***\*\*\*\***\*\***\*\*\*\***\***\*\*\*\***\*\***\*\*\*\***\*\*\*\***\*\*\*\***\*\***\*\*\*\***
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a kinda strange way to set comments are you sure about that? this comment applies to all mdx files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is auto generated after running yarn pretty

Copy link
Contributor

@evegufy evegufy left a comment

Choose a reason for hiding this comment

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

@evegufy
Copy link
Contributor

evegufy commented Jun 24, 2024

IP Issues created:
A review is required for npm/npmjs/@storybook/types/8.1.10.
A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15486 .
A review is required for npm/npmjs/@vitest/utils/1.6.0.
A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15487 .
A review is required for npm/npmjs/@storybook/addon-interactions/8.1.10.
A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15483 .
A review is required for npm/npmjs/@storybook/manager/8.1.10.
A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15484 .
A review is required for npm/npmjs/@storybook/addon-onboarding/8.1.10.
A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15485 .
A review is required for npm/npmjs/-/chromatic/11.5.4.
A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15478 .
A review is required for npm/npmjs/@chromatic-com/storybook/1.5.0.
A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15479 .
A review is required for npm/npmjs/-/prettier/3.3.2.
A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15480 .
A review is required for npm/npmjs/@mui/x-data-grid/6.20.1.
A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15481 .
A review is required for npm/npmjs/-/eslint-plugin-es-x/7.7.0.
A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15482 .

@manojava-gk
Copy link
Contributor Author

manojava-gk commented Jun 25, 2024

Please also fix failing linter https://github.com/eclipse-tractusx/portal-shared-components/actions/runs/9611276695/job/26509514085?pr=210

There is no issue in the local, when I run yarn lint, but here it is failing

@manojava-gk manojava-gk requested a review from oyo June 28, 2024 09:14
@manojava-gk manojava-gk marked this pull request as ready for review June 28, 2024 11:30
@oyo
Copy link
Contributor

oyo commented Jul 12, 2024

Please also fix failing linter https://github.com/eclipse-tractusx/portal-shared-components/actions/runs/9611276695/job/26509514085?pr=210

There is no issue in the local, when I run yarn lint, but here it is failing

For me it is also failing on local:

 portal-shared-components yarn lint
yarn run v1.22.22
$ eslint . --ext ts,tsx,js,jsx --report-unused-disable-directives --max-warnings 0
There was a problem loading formatter: /Users/q163684/work/eclipse-tractusx/portal-shared-components/node_modules/eslint/lib/cli-engine/formatters/stylish
Error: require() of ES Module /Users/q163684/work/eclipse-tractusx/portal-shared-components/node_modules/strip-ansi/index.js from /Users/q163684/work/eclipse-tractusx/portal-shared-components/node_modules/eslint/lib/cli-engine/formatters/stylish.js not supported.
Instead change the require of index.js in /Users/q163684/work/eclipse-tractusx/portal-shared-components/node_modules/eslint/lib/cli-engine/formatters/stylish.js to a dynamic import() which is available in all CommonJS modules.
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@manojava-gk
Copy link
Contributor Author

Please also fix failing linter https://github.com/eclipse-tractusx/portal-shared-components/actions/runs/9611276695/job/26509514085?pr=210

There is no issue in the local, when I run yarn lint, but here it is failing

For me it is also failing on local:

 portal-shared-components yarn lint
yarn run v1.22.22
$ eslint . --ext ts,tsx,js,jsx --report-unused-disable-directives --max-warnings 0
There was a problem loading formatter: /Users/q163684/work/eclipse-tractusx/portal-shared-components/node_modules/eslint/lib/cli-engine/formatters/stylish
Error: require() of ES Module /Users/q163684/work/eclipse-tractusx/portal-shared-components/node_modules/strip-ansi/index.js from /Users/q163684/work/eclipse-tractusx/portal-shared-components/node_modules/eslint/lib/cli-engine/formatters/stylish.js not supported.
Instead change the require of index.js in /Users/q163684/work/eclipse-tractusx/portal-shared-components/node_modules/eslint/lib/cli-engine/formatters/stylish.js to a dynamic import() which is available in all CommonJS modules.
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@oyo I tried from my end. I do not see any error in local

@manojava-gk
Copy link
Contributor Author

@evegufy Any update on Dependency file?

@manojava-gk manojava-gk requested a review from evegufy July 16, 2024 11:15
@evegufy
Copy link
Contributor

evegufy commented Jul 16, 2024

IP Issues created: A review is required for npm/npmjs/@storybook/types/8.1.10. A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15486 . A review is required for npm/npmjs/@vitest/utils/1.6.0. A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15487 . A review is required for npm/npmjs/@storybook/addon-interactions/8.1.10. A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15483 . A review is required for npm/npmjs/@storybook/manager/8.1.10. A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15484 . A review is required for npm/npmjs/@storybook/addon-onboarding/8.1.10. A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15485 . A review is required for npm/npmjs/-/chromatic/11.5.4. A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15478 . A review is required for npm/npmjs/@chromatic-com/storybook/1.5.0. A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15479 . A review is required for npm/npmjs/-/prettier/3.3.2. A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15480 . A review is required for npm/npmjs/@mui/x-data-grid/6.20.1. A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15481 . A review is required for npm/npmjs/-/eslint-plugin-es-x/7.7.0. A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15482 .

@manojava-gk I created those I issues and they appear to be closed, did you change the versions? If so, change them back.

@manojava-gk
Copy link
Contributor Author

manojava-gk commented Jul 17, 2024

https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15486

@evegufy I have not added any new packages here. I cleaned up my local and generated new yarn.lock file by running yarn install. pr checks used to get failed with the older version but now it is fixed

@oyo you want to add anything here?

@evegufy
Copy link
Contributor

evegufy commented Jul 17, 2024

https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15486

@evegufy I have not added any new packages here. I cleaned up my local and generated new yarn.lock file by running yarn install. pr checks used to get failed with the older version but now it is fixed

@oyo you want to add anything here?

You did change the versions of the packages very evidently from 7323404 - which is when I created the IP issues - to f9891d8, what made the created IP issues useless for this pull request.

I had to create now new IP issues:
A review is required for npm/npmjs/@chromatic-com/storybook/1.6.1.
A review request already exists https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15710
A review is required for npm/npmjs/@mui/icons-material/5.16.2.
A review request already exists https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15711
A review is required for npm/npmjs/-/eslint-plugin-es-x/7.8.0.
A review request already exists https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15712
A review is required for npm/npmjs/-/esquery/1.6.0.
A review request already exists https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15713
A review is required for npm/npmjs/@mui/material/5.16.2.
A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15714
A review is required for npm/npmjs/@mui/system/5.16.2.
A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15715
A review is required for npm/npmjs/@mui/utils/5.16.2.
A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15716
A review is required for npm/npmjs/@mui/private-theming/5.16.2.
A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15717
A review is required for npm/npmjs/@mui/styled-engine/5.16.2.
A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15718
A review is required for npm/npmjs/@testing-library/dom/10.1.0.
A review request already exists https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15719
A review is required for npm/npmjs/@mui/x-data-grid/6.20.3.
A review request already exists https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15720
A review is required for npm/npmjs/@storybook/core/8.2.4.
A review request already exists https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15721

I assume some of them will ran again in a manual review, so expect a wait time from at leat two weeks.

Copy link
Contributor

@evegufy evegufy left a comment

Choose a reason for hiding this comment

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

Change title of pull request, it doesn't summarise the contents well

@manojava-gk manojava-gk changed the title feat(storybook): support mdx feat(storybook): upgrade story book to support mdx files Jul 17, 2024
@evegufy evegufy self-requested a review July 17, 2024 07:12
@manojava-gk
Copy link
Contributor Author

https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15486

@evegufy I have not added any new packages here. I cleaned up my local and generated new yarn.lock file by running yarn install. pr checks used to get failed with the older version but now it is fixed
@oyo you want to add anything here?

You did change the versions of the packages very evidently from 7323404 - which is when I created the IP issues - to f9891d8, what made the created IP issues useless for this pull request.

I had to create now new IP issues: A review is required for npm/npmjs/@chromatic-com/storybook/1.6.1. A review request already exists https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15710 A review is required for npm/npmjs/@mui/icons-material/5.16.2. A review request already exists https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15711 A review is required for npm/npmjs/-/eslint-plugin-es-x/7.8.0. A review request already exists https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15712 A review is required for npm/npmjs/-/esquery/1.6.0. A review request already exists https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15713 A review is required for npm/npmjs/@mui/material/5.16.2. A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15714 A review is required for npm/npmjs/@mui/system/5.16.2. A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15715 A review is required for npm/npmjs/@mui/utils/5.16.2. A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15716 A review is required for npm/npmjs/@mui/private-theming/5.16.2. A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15717 A review is required for npm/npmjs/@mui/styled-engine/5.16.2. A review request was created https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15718 A review is required for npm/npmjs/@testing-library/dom/10.1.0. A review request already exists https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15719 A review is required for npm/npmjs/@mui/x-data-grid/6.20.3. A review request already exists https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15720 A review is required for npm/npmjs/@storybook/core/8.2.4. A review request already exists https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15721

I assume some of them will ran again in a manual review, so expect a wait time from at leat two weeks.

we do not manually change the yarn.lock file. It is auto generated based on package.json and currently available version from the internet. It got changed as the new version is available in the internet.

@evegufy
Copy link
Contributor

evegufy commented Jul 30, 2024

IP issues currently still under review:
npm/npmjs/@mui/x-data-grid/6.20.3
https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15720
npm/npmjs/@storybook/core/8.2.4
https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15721

@catenax-ng catenax-ng closed this by deleting the head repository Jul 31, 2024
@evegufy evegufy reopened this Aug 5, 2024
Copy link

sonarcloud bot commented Aug 5, 2024

@evegufy
Copy link
Contributor

evegufy commented Aug 5, 2024

Hi @manojava-gk as the catenax-ng GitHub org was recreated, I was able to reopen the PR... please reopen the PR as soon as possible from your own fork as the catenax-ng GitHub org will be deleted again.
could it be possible to contribute this change in multiple pull requests? this one got quite huge
BTW, IP issues are all approved now.

@oyo oyo merged commit cf80f1d into eclipse-tractusx:main Aug 12, 2024
8 of 9 checks passed
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.

5 participants