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

refactor(sbb-calendar): implement initial support for other date libraries #2511

Merged
merged 4 commits into from
Mar 21, 2024

Conversation

kyubisation
Copy link
Contributor

@kyubisation kyubisation commented Mar 20, 2024

This change allows us in the future to support different date libraries. However, this is only the implementation for the calendar. Datepicker will be done in a different PR.

BREAKING CHANGE: The SbbDatepicker property selectedDate has been renamed to selected. This also applies to the attribute selected-date, which has been renamed to selected. Additionally the DateAdapter (and NativeDateAdapter) have been superficially refactored. An important change is that the month is now 1-based, instead of 0-based.

…aries

BREAKING CHANGE:
The `SbbDatepicker` property `selectedDate` has been renamed to `selected`.
This also applies to the attribute `selected-date`, which has been renamed to `selected`.
Additionally the `DateAdapter` (and `NativeDateAdapter`) have been superficially refactored.
An important change is that the month is now `1`-based, instead of `0`-based.
@github-actions github-actions bot added the pr: peer review required A peer review is required for this pull request label Mar 20, 2024
Copy link
Contributor

@jeripeierSBB jeripeierSBB left a comment

Choose a reason for hiding this comment

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

As it is fairly complex, it's difficult to determine wether everything is fully like expected :). But generally looks good.
As testing fails, there is so far no build to manually experiment with the changes.

src/components/calendar/calendar.ts Outdated Show resolved Hide resolved
src/components/calendar/calendar.ts Outdated Show resolved Hide resolved
src/components/calendar/calendar.ts Outdated Show resolved Hide resolved
src/components/calendar/calendar.ts Outdated Show resolved Hide resolved
src/components/calendar/calendar.ts Outdated Show resolved Hide resolved
src/components/calendar/calendar.ts Outdated Show resolved Hide resolved
src/components/core/datetime/date-adapter.ts Outdated Show resolved Hide resolved
src/components/core/datetime/date-adapter.ts Show resolved Hide resolved
src/components/calendar/calendar.e2e.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to preview-pr2511 March 21, 2024 09:21 Inactive
Copy link
Contributor

@jeripeierSBB jeripeierSBB left a comment

Choose a reason for hiding this comment

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

Codewise ok, but see chromatic changes, something with disabling buttons doesn't work anymore? or not the same?

@codecov-commenter
Copy link

codecov-commenter commented Mar 21, 2024

Codecov Report

Attention: Patch coverage is 90.75145% with 32 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (main@3eefbea). Click here to learn what that means.

Files Patch % Lines
src/components/core/datetime/date-adapter.ts 86.36% 18 Missing ⚠️
src/components/calendar/calendar.ts 91.71% 14 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2511   +/-   ##
=======================================
  Coverage        ?   93.85%           
=======================================
  Files           ?      299           
  Lines           ?    24495           
  Branches        ?     2021           
=======================================
  Hits            ?    22990           
  Misses          ?     1476           
  Partials        ?       29           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot temporarily deployed to preview-pr2511 March 21, 2024 12:00 Inactive
Copy link
Contributor

@jeripeierSBB jeripeierSBB left a comment

Choose a reason for hiding this comment

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

LGTM. thanks for the effort!

@github-actions github-actions bot added the pr: lead review approved Pull request has been approved by a lead review label Mar 21, 2024
Copy link
Contributor

@MarioCastigliano MarioCastigliano left a comment

Choose a reason for hiding this comment

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

Love the getter/setter approach on the calendar 👍

@github-actions github-actions bot added pr: peer review approved Pull request has been approved by a peer review and removed pr: peer review required A peer review is required for this pull request labels Mar 21, 2024
@kyubisation kyubisation merged commit 6d4e9c2 into main Mar 21, 2024
13 of 14 checks passed
@kyubisation kyubisation deleted the refactor-calendar branch March 21, 2024 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: lead review approved Pull request has been approved by a lead review pr: peer review approved Pull request has been approved by a peer review preview-available
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants