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

feat(sbb-calendar): vertical orientation #3378

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

feat(sbb-calendar): vertical orientation #3378

wants to merge 15 commits into from
+1,200 −406

Conversation

DavideMininni-Fincons
Copy link
Contributor

Preflight Checklist

Issue

This PR Closes #3336

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

See Review Guidelines for more information what is checked during review process.

Changes

Changes in this pull request:

  • a new parameter called orientation has been added on the sbb-calendar, which accept horizontal (default) and vertical as values;
  • implementation of vertical orientation;
  • fix bug in keyboard navigation in wide mode in month view;
  • refactoring of the whole logic of the keyboard navigation in day view, 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, the data-day attribute has been replaced with the value which is filled with the day's corresponding date as ISO string. Keyboard navigation is now a date calculation;
  • related to previous point, fix on the logic of the firstFocusable calculation;
  • improvements on docs and tests

Browsers

I tested the build on the following browsers:

  • Firefox Desktop
  • Chrome Desktop
  • Edge Desktop
  • Safari Desktop
  • Chrome Mobile
  • Safari Mobile

Screen readers

I tested the build on the following browsers:

  • JAWS Firefox Desktop
  • JAWS Chrome Desktop
  • NVDA Firefox Desktop
  • NVDA Chrome Desktop
  • VoiceOver Safari Desktop
  • VoiceOver Chrome Desktop
  • VoiceOver Safari Mobile
  • Android Accessibility Suite Chrome Mobile

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Does this introduce a breaking change?

  • Yes
  • No

Other information

@DavideMininni-Fincons DavideMininni-Fincons linked an issue Jan 31, 2025 that may be closed by this pull request
story(sbb-calendar): vertical orientation #3336
Open
9 tasks
@github-actions github-actions bot added target: 2.x pr: peer review required A peer review is required for this pull request labels Jan 31, 2025
@github-actions github-actions bot requested a deployment to pr3378 January 31, 2025 10:20 In progress
@github-actions github-actions bot requested a deployment to pr3378-diff January 31, 2025 10:20 In progress
MarioCastigliano
MarioCastigliano reviewed Feb 4, 2025
View reviewed changes
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.

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

src/elements/calendar/calendar.ts
@@ -999,11 +1304,11 @@ class SbbCalendarElement<T = Date> extends SbbHydrationMixin(LitElement) {
})}
@click=${() => this._selectDate(day.value)}
?disabled=${isOutOfRange || isFilteredOut}
value=${day.value}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a proper use for the button's value attribute, I feel like a custom attribute would be more suited for this scenario

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarioCastigliano MarioCastigliano MarioCastigliano left review comments

@TomMenga TomMenga Awaiting requested review from TomMenga

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

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

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
diff-available pr: peer review required A peer review is required for this pull request pr: visual review required preview-available target: 2.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

story(sbb-calendar): vertical orientation
2 participants
@DavideMininni-Fincons @MarioCastigliano