feat(sbb-pearl-chain): implement new component structure #3172
Conversation
github-actions
bot
added
pr: visual review required
diff-available
and removed
preview-available
diff-available
labels
MarioCastigliano
added
the
pr: peer review required
src/elements/pearl-chain/pearl-chain-leg/pearl-chain-leg.visual.spec.ts
Outdated
Show resolved
Hide resolved
|
|
||
| ## States | ||
|
|
||
| The `past`, `arrival-skipped`, `departure-skipped`, and `disruption` properties are connected to the self-named states. |
There was a problem hiding this comment.
Some extra info about what happens when these params are set would be helpful (e.g. what is rendered when)
src/elements/pearl-chain/pearl-chain/pearl-chain.visual.spec.ts
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,107 @@ | |||
| import { html } from 'lit'; | |||
There was a problem hiding this comment.
Forced colors tests?
src/elements/pearl-chain/pearl-chain/pearl-chain.sample-data.ts
Outdated
Show resolved
Hide resolved
github-actions
bot
added
diff-available
and removed
preview-available
diff-available
labels
MarioCastigliano
requested review from
kyubisation and
jeripeierSBB
as code owners
| } | ||
|
|
||
| private _removeTimezone(time: SbbDateLike | null): Date | undefined { | ||
| const parsedDate = defaultDateAdapter.deserialize(time); |
There was a problem hiding this comment.
dateAdapter should be loaded from config:
private _dateAdapter: DateAdapter<T> = readConfig().datetime?.dateAdapter ?? defaultDateAdapter;
(every defaultDateAdapter should be replaced with that) could also have influence of other date calculations
| height: var(--sbb-pearl-chain-height); | ||
| border-inline-end: var(--sbb-pearl-chain-last-leg-margin, var(--sbb-pearl-chain-spacing-small)) | ||
| solid Canvas; | ||
| background-color: currentcolor; |
There was a problem hiding this comment.
Basic question: why do you define color and then define background with currentcolor? as there is no text at all in this component, couldn't we directly style the background?
|
|
||
| @mixin sbb-pearl-chain-dotted { | ||
| background-color: unset; | ||
| background-image: linear-gradient(to right, currentcolor 0%, currentcolor 50%, Canvas 50%); |
There was a problem hiding this comment.
Could you remove all Canvas or white background usages and replace them with another approach? I know it's not that easy, but I guess it should be possible. (e.g. with mask-image and linear gradient).
Using Canvas prints a white background:
| content: ''; | ||
| position: absolute; | ||
| inset-block-start: -200%; | ||
| z-index: 4; |
There was a problem hiding this comment.
as other comments, refactoring without z-index possible?
|
|
||
| import './pearl-chain-leg.js'; | ||
|
|
||
| describe('sbb-pearl-chain-leg', () => { |
There was a problem hiding this comment.
Should we also test combinations of props? like past and disruption and so on... so we ensure everyhing looks like expected.
|
|
||
| import './pearl-chain-leg.js'; | ||
|
|
||
| describe('sbb-pearl-chain-leg', () => { |
There was a problem hiding this comment.
I recommend to share the template either with cases and describeEach, or just a beforEach(). now e.g. every departure and arrival time is the same (times don't affect rendering right now)
jeripeierSBB
changed the title
jeripeierSBB
changed the title
TomMenga
added
the
pr: lead review required
MarioCastigliano
force-pushed
the
refactor/pearl-chain
branch
from
1a7cc72
to
77f0d3c
Compare
Preflight Checklist
Issue
This PR Closes #2335
Pull request checklist
Please check if your PR fulfills the following requirements:
See Review Guidelines for more information what is checked during review process.
Changes
Changes in this pull request:
Browsers
I tested the build on the following browsers:
Screen readers
I tested the build on the following browsers:
Pull request type
Please check the type of change your PR introduces:
Does this introduce a breaking change?
Other information