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(sbb-train-formation): introduce new types and refactoring #3199

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

jeripeierSBB
Copy link
Contributor

@jeripeierSBB jeripeierSBB commented Nov 6, 2024

Closes #2681

BREAKING CHANGE: The hide-wagen-label property of the sbb-train-formation was removed. Now it automatically doesn't show the label if no label is set on all the wagons.
The i18n i18nClosedCompartmentLabel() method doesn't take wagonNumber as an argument anymore but is a constant now.
Additionally, there are some visual changes:

  • sbb-train-wagon: The ouccpancy property doesn't default to none anymore but to null. Please replace the currently undefined occupancy property with the value none.
  • sbb-train-wagon: Previously for the locomotive the label was not displayed, but now it would, as soon as there is one provided
  • sbb-train-formation: The inline padding (left / right) was removed but can be set by CSS variable. See documentation.

@github-actions github-actions bot added the pr: peer review required A peer review is required for this pull request label Nov 6, 2024
@github-actions github-actions bot temporarily deployed to pr3199 November 6, 2024 16:21 Inactive
@github-actions github-actions bot temporarily deployed to pr3199-diff November 6, 2024 16:22 Inactive
@jeripeierSBB jeripeierSBB changed the title Train formation update feat(sbb-train-formation): introduce new types and refactoring Nov 6, 2024
@github-actions github-actions bot temporarily deployed to pr3199 November 7, 2024 11:02 Inactive
@github-actions github-actions bot temporarily deployed to pr3199-diff November 7, 2024 11:03 Inactive
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.

Generally LGTM 👍
Some questions

src/elements/train/train-wagon/readme.md Outdated Show resolved Hide resolved
src/elements/train/train-wagon/train-wagon.ts Outdated Show resolved Hide resolved
src/elements/train/train/train.ts Outdated Show resolved Hide resolved
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.

LGTM 👍
Nice work! 😃


@include sbb.if-forced-colors {
--sbb-train-formation-sector-line-color: CanvasText;
}
}

:host([hide-wagon-label]) {
--sbb-train-formation-wagon-label-display: none;
// TODO: Move to sbb-train-wagon after CSS refactoring
Copy link
Contributor

@kyubisation kyubisation Nov 13, 2024

Choose a reason for hiding this comment

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

Probably move to global CSS with the CSS refactoring, as we don't want to repeat these SVGs in each SSR rendered component

}
</${unsafeStatic(TAG_NAME)}>
`;
const typeLabel = (): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick; Could probably be extracted into a private method?

@github-actions github-actions bot added the pr: lead review approved Pull request has been approved by a lead review label Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

[Change request]: changes for sbb-train-wagon (type "closed", sleeping car/couchette)
2 participants