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

chore: migrate design system docs to Vue 3 #10243

Merged
merged 9 commits into from
Jan 18, 2024
Merged

Conversation

LukasHirt
Copy link
Collaborator

@LukasHirt LukasHirt commented Dec 27, 2023

Description

  • Upgrade vue styleguidist
  • Upgrade webpack
  • Upgrade codemirror
  • Move internal docs components into design system components as private due to an issue where loading it from multiple folders results in memory errors during build
  • Migrate the preview code editor to the new version of codemirror
  • Create new vue app in preview in order to extend it with e.g. gettext (ugly solution but styleguidist does not seem offer a way to extend the existing vue app)

Related Issue

Motivation and Context

Make design system docs buildable again and update dependencies

How Has This Been Tested?

  • test environment: local
  • test case 1: run docs locally

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • fix issues with style loaders
  • fix issue with memory when using base webpack config
  • fix missing scss vars
  • make sure all sections are still rendering as they should
  • fix lint issues

Copy link

update-docs bot commented Dec 27, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@kulmann kulmann mentioned this pull request Jan 2, 2024
@LukasHirt LukasHirt force-pushed the chore/design-docs-update branch 2 times, most recently from f254fcb to a338c3d Compare January 4, 2024 17:47
@LukasHirt LukasHirt force-pushed the chore/design-docs-update branch from d4877a0 to 7f4c595 Compare January 12, 2024 21:09
@LukasHirt LukasHirt force-pushed the chore/design-docs-update branch from e2d5a32 to d1badec Compare January 15, 2024 17:59
@LukasHirt LukasHirt marked this pull request as ready for review January 15, 2024 18:23
@LukasHirt
Copy link
Collaborator Author

@JammingBen PR is ready for review.

Let me pls know how do you want to handle changelog here. Also, with @kulmann we agreed to do tests in separate PR to quickly move on with dependency warnings but sonarcloud fails here due to insufficient coverage of new changes... let me pls know how do you want to handle this as well. I will look into the other sonarcloud issues in the meantime.

Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

First off: awesome stuff, really great to finally have the ODS running again! 🥳

Let me pls know how do you want to handle changelog here.

I'd say this is a bugfix inside the design system's changelog folder, since building the ODS was basically broken before.

Also, with @kulmann we agreed to do tests in separate PR to quickly move on with dependency warnings but sonarcloud fails here due to insufficient coverage of new changes... let me pls know how do you want to handle this as well.

SonarCloud reports weird things sometimes (actually quite often), so it's not a blocker for our PRs in general. You can fix valid reports in a follow-up as well IMO.

I see that some unit tests for the docs are failing. I have no problem if you remove them to be honest, I don't really see the point in unit testing doc pages.

2 issues I still have when testing the build on my local machine:

  • When running pnpm build:docs I get errors reporting a missing mini-css-extract-plugin plugin (see annotation down below).
  • When running pnpm start I get some sass errors reporting undefined variables.

Copy link
Member

@dschmidt dschmidt left a comment

Choose a reason for hiding this comment

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

More or less nitpicks but I basically agree with everything that Jannik already said.

Additionally I would like to request that we have at least one basic e2e test, that makes sure the documentation app boots, we can navigate towards the documentation of a component and check it's rendered and functional (e.g. go to OcRadio and change the state)
That would give us much more confidence when accepting renovatebot PRs

Great work already!

@LukasHirt
Copy link
Collaborator Author

When running pnpm start I get some sass errors reporting undefined variables.

This is basically happening because there are same tokens defined for docs and the design system and the build just drops the docs ones... I'll see if the overwrite can be prevented.

@LukasHirt
Copy link
Collaborator Author

Additionally I would like to request that we have at least one basic e2e test

Yep, this is planned for the follow up PR.

@LukasHirt LukasHirt force-pushed the chore/design-docs-update branch from 66da328 to fe83bdf Compare January 17, 2024 12:52
@LukasHirt
Copy link
Collaborator Author

All issues resolved - ready for second round @JammingBen @dschmidt

Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

Pushed a little fix for the initial focus on modals (which was broken for some reason 🤷 ).

Also I noticed that the examples in the docs are empty (between a component's description and the code block). But I'd say let's merge this PR as a first step and iterate from here 👍

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions

0.0% Condition Coverage on New Code (required ≥ 50%)

See analysis details on SonarCloud

Copy link
Member

@dschmidt dschmidt left a comment

Choose a reason for hiding this comment

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

I'm happy with this as a first iteration, but I would really like integration in our CI pipeline for building the ODS docs and having basic e2e tests (the latter possibly as a third iteration)

@dschmidt dschmidt merged commit 1f9946e into master Jan 18, 2024
1 check failed
@delete-merged-branch delete-merged-branch bot deleted the chore/design-docs-update branch January 18, 2024 10:59
ownclouders pushed a commit that referenced this pull request Jan 18, 2024
* chore: upgrade vue styleguidist

* Fix acorn issue

* Downgrade vue in design-system

* chore: move docs components together with the rest and migrate codemirror

* refactor: conform to eslint rules

* test: update snapshots and use test utils instead of vue constructor

* refactor: conform to sonarcloud standards

* fix: bring back minicss plugin, resolve tokens collisions and drop docs unit tests

* fix: initial modal focus

---------

Co-authored-by: Dominik Schmidt <[email protected]>
Co-authored-by: Jannik Stehle <[email protected]>
@JammingBen JammingBen mentioned this pull request Jan 19, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants