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.

Sign up for GitHub

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

Jump to bottom

refactor: rename lyne-components into lyne-elements and create lyne-elements-experimental #2675

Merged
merged 21 commits into from
May 29, 2024
Merged

refactor: rename lyne-components into lyne-elements and create lyne-elements-experimental #2675

merged 21 commits into from
May 29, 2024

Conversation

TomMenga
Copy link
Contributor

@TomMenga TomMenga commented May 21, 2024 •
edited
Loading

Brief description

This PR Closes #2617

We now have the following libraries:

  • @sbb-esta/lyne-elements => src/elements
  • @sbb-esta/lyne-elements-experimental => src/elements-experimental (has a dependency to @sbb-esta/lyne-elements)
  • @sbb-esta/lyne-react => src/react (has a dependency to @sbb-esta/lyne-elements)
  • @sbb-esta/lyne-react-experimental => src/react-experimental (has a dependency to @sbb-esta/lyne-elements-experimental and @sbb-esta/lyne-elements)

Topics to discuss:

  • I marked the inter-dependencies as peer dependency. That forces the user to explicitly have them in their package.json. I feel like it's better this way (also cdk does it). What do you think?
  • Adapt the CI/CD (probably just the release-please)

BEGIN_COMMIT_OVERRIDE
refactor: rename lyne-components into lyne-elements and create lyne-elements-experimental

BREAKING CHANGE: Lyne components and Lyne components react libraries have been renamed, please update imports accordingly.

  • @sbb-esta/lyne-components => @sbb-esta/lyne-elements
  • @sbb-esta/lyne-components-react => @sbb-esta/lyne-react
    The following components, and their react wrappers, have been moved into @sbb-esta/lyne-elements-experimental and @sbb-esta/lyne-react-experimental respectively:
  • sbb-journey-summary
  • sbb-pearl-chain
  • sbb-pearl-chain-time
  • sbb-pearl-chain-vertical
  • sbb-pearl-chain-vertical-item
  • sbb-timetable-duration
  • sbb-timetable-row

END_COMMIT_OVERRIDE

@github-actions github-actions bot added the pr: peer review required A peer review is required for this pull request label May 21, 2024
@TomMenga TomMenga added the pr: lead review required A lead review is required for this pull request label May 21, 2024
@TomMenga TomMenga self-assigned this May 21, 2024
@github-actions github-actions bot temporarily deployed to pr2675 May 21, 2024 09:45 Inactive
@github-actions github-actions bot temporarily deployed to pr2675 May 23, 2024 08:05 Inactive
@github-actions github-actions bot temporarily deployed to pr2675 May 23, 2024 10:37 Inactive
@github-actions github-actions bot temporarily deployed to pr2675 May 27, 2024 10:36 Inactive
@kyubisation kyubisation mentioned this pull request May 28, 2024
Merged
kyubisation
kyubisation reviewed May 28, 2024
View reviewed changes
Copy link
Contributor

@kyubisation kyubisation left a comment

Choose a reason for hiding this comment

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

Excellent work! 😃

docs/CODING_STANDARDS.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Show resolved Hide resolved
src/elements/tsconfig.json Outdated Show resolved Hide resolved
src/elements/core/datetime/date-helper.ts Outdated Show resolved Hide resolved
src/react/experimental-vite.config.ts Outdated Show resolved Hide resolved
@kyubisation
Copy link
Contributor

Additional thought; Should we add an explanation for the experimental packages to the root README? And maybe a quick additional section to the GETTING_STARTED.md?

@TomMenga
Copy link
Contributor Author

Additional thought; Should we add an explanation for the experimental packages to the root README? And maybe a quick additional section to the GETTING_STARTED.md?

I might need some input on the exact reason why we created the experimental package cause I'm not sure myself (I can guess it, but not sure what's the official reason).

@TomMenga
Merge branch 'refs/heads/main' into refactor/split-packages
6a4246e 
# Conflicts:
#	src/elements/core/testing/private/load-asset-as-base64.ts
#	src/elements/lead-container.ts
#	src/elements/lead-container/__snapshots__/lead-container.snapshot.spec.snap.js
#	src/elements/lead-container/assets/lucerne.png
#	src/elements/lead-container/lead-container.scss
#	src/elements/lead-container/lead-container.snapshot.spec.ts
#	src/elements/lead-container/lead-container.stories.ts
#	src/elements/lead-container/lead-container.ts
#	src/elements/lead-container/readme.md
@github-actions github-actions bot temporarily deployed to pr2675 May 28, 2024 15:58 Inactive
@kyubisation
Copy link
Contributor

Suggestion: "The experimental packages provide components that have either not yet been battle tested or do not yet align with our architecture or testing goals. Once they become stable, we will move them into the main packages."

@TomMenga
Copy link
Contributor Author

Additional thought; Should we add an explanation for the experimental packages to the root README? And maybe a quick additional section to the GETTING_STARTED.md?

Docs updated 😸
I took the liberty of slightly changing the suggestion, see if you like it (took inspiration from angular/components) @kyubisation @jeripeierSBB

@TomMenga TomMenga requested a review from kyubisation May 29, 2024 09:56
@github-actions github-actions bot added pr: lead review approved Pull request has been approved by a lead review and removed pr: lead review required A lead review is required for this pull request labels May 29, 2024
kyubisation
kyubisation approved these changes May 29, 2024
View reviewed changes
Copy link
Contributor

@kyubisation kyubisation left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 👍
Thank you very much!

@TomMenga TomMenga merged commit edd3a73 into main May 29, 2024
28 of 30 checks passed
@TomMenga TomMenga deleted the refactor/split-packages branch May 29, 2024 11:04
@github-actions github-actions bot added pr: peer review required A peer review is required for this pull request and removed pr: peer review required A peer review is required for this pull request labels May 29, 2024
@github-actions github-actions bot mentioned this pull request May 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kyubisation kyubisation kyubisation approved these changes

@jeripeierSBB jeripeierSBB Awaiting requested review from jeripeierSBB jeripeierSBB is a code owner

Assignees

@TomMenga TomMenga

Labels
diff-available pr: lead review approved Pull request has been approved by a lead review pr: peer review required A peer review is required for this pull request pr: visual review required preview-available
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Split packages into lyne-elements, lyne-elements-experimental, lyne-react and lyne-react-experimental
2 participants
@TomMenga @kyubisation