feat(sbb-calendar): vertical orientation #3378
Conversation
DavideMininni-Fincons
requested review from
kyubisation and
jeripeierSBB
as code owners
github-actions
bot
added
target: 2.x
pr: peer review required
There was a problem hiding this comment.
Great job. Just a consideration about the fact that the width of the component depends on which month is currently in view, like in horizontal mode the height was changing, but in this case it can happen that you kinda have to chase the next month arrow around. Should this be addressed somehow? (maybe an empty column when necessary)
Also I think I might have a suggestion for a more efficient way to render the tables, but since it would have been a million comments (and I'm not 100% sure it would work) it's easier for me to try to implement it and show it to you
jeripeierSBB
added
the
pr: ux review required
There was a problem hiding this comment.
Great work!
There are some odd looking visual tests:
- https://lyne-components-visual-regression-diff-pr3378.app.sbb.ch/compare/sbb-calendar/orientation=vertical_wide=true_dynamic_width
- https://lyne-components-visual-regression-diff-pr3378.app.sbb.ch/compare/sbb-calendar/orientation=vertical_wide=false_dynamic_width
- https://lyne-components-visual-regression-diff-pr3378.app.sbb.ch/compare/sbb-calendar/orientation=horizontal_wide=true_dynamic_width
- https://lyne-components-visual-regression-diff-pr3378.app.sbb.ch/compare/sbb-calendar/orientation=horizontal_wide=false_dynamic_width
| @@ -266,117 +275,236 @@ describe(`sbb-calendar`, () => { | |||
| ).to.be.equal('2022'); | |||
| }); | |||
|
|
|||
| describe('navigation', () => { | |||
| it('navigates left via keyboard', async () => { | |||
| describe('keyboard navigation', () => { | |||
There was a problem hiding this comment.
I think it's a little bit more readable if the test names can be read with "it ....", either like "it should focus on the selected...", or "it focuses on the selected date...". Similar for other tests.
| value: string; | ||
| dayValue: string; | ||
| monthValue: string; | ||
| yearValue: string; | ||
| dateValue: T; |
There was a problem hiding this comment.
This is a (soft) breaking change (generic type and required dateValue). Should we merge this or not? @kyubisation
| private _handleKeyboardEvent(event: KeyboardEvent, day?: Day): void { | ||
| /** | ||
| * In `day` view in `vertical` orientation, | ||
| * if the first of the month is not a Monday, it is not the first rendered element is the table, |
There was a problem hiding this comment.
Probably in?
| * if the first of the month is not a Monday, it is not the first rendered element is the table, | |
| * if the first of the month is not a Monday, it is not the first rendered element in the table, |
| @@ -192,6 +199,15 @@ export abstract class DateAdapter<T = any> { | |||
| return `${pad(this.getYear(date), 4)}-${pad(this.getMonth(date))}-${pad(this.getDate(date))}`; | |||
| } | |||
|
|
|||
| /** | |||
| * Retruns a date from ISO String. | |||
There was a problem hiding this comment.
| * Retruns a date from ISO String. | |
| * Returns a date from ISO String. |
| * Retruns a date from ISO String. | ||
| * @param date The ISO String date to convert to T. | ||
| */ | ||
| public fromIso8601(date: string): T { |
There was a problem hiding this comment.
I think this method is already covered by deserialize() and should be removed here.
| * Gets the date as milliseconds since epoch. | ||
| * @param date | ||
| */ | ||
| public abstract getTime(date: T): number; |
There was a problem hiding this comment.
time should not be part of a date adapter. is there an alternative solution?
| @@ -34,6 +34,11 @@ export class NativeDateAdapter extends DateAdapter<Date> { | |||
| return date.getDate(); | |||
| } | |||
|
|
|||
| /** Gets the date as milliseconds since epoch. */ | |||
There was a problem hiding this comment.
See comment in date-adapter
mcilurzo
added
pr: ux review approved
|
looks good to me. |
Preflight Checklist
Issue
This PR Closes #3336
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:
orientationhas been added on thesbb-calendar, which accepthorizontal(default) andverticalas values;monthview;dayview, since it relies on calculation on the elements position (index of the element in the array of rendered cells). With vertical orientation the first rendered element is always the first Monday of the month and not the first day of the month. To fix this, thedata-dayattribute has been replaced with thevaluewhich is filled with the day's corresponding date as ISO string. Keyboard navigation is now a date calculation;firstFocusablecalculation;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