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(ui5-daterange-picker): keyboard handling improvement #2179

Merged
merged 22 commits into from
Oct 9, 2020

Conversation

unazko
Copy link
Contributor

@unazko unazko commented Sep 4, 2020

Depending on the caret symbol position, the corresponding date gets incremented
or decremented with one unit of measure by using the following keyboard
combinations:

[PAGEDOWN] - Decrements the corresponding day of the month by one
[SHIFT] + [PAGEDOWN] - Decrements the corresponding month by one
[SHIFT] + [CTRL] + [PAGEDOWN] - Decrements the corresponding year by one
[PAGEUP] - Increments the corresponding day of the month by one
[SHIFT] + [PAGEUP] - Increments the corresponding month by one
[SHIFT] + [CTRL] + [PAGEUP] - Increments the corresponding year by one

Related to: #1534

The button should receive a correct focus outline
when the 'tab' key is pressed.
Depending on the caret symbol position, the corresponding date gets incremented
or decremented with one unit of measure by using the following keyboard
combinations:

[PAGEDOWN] - Decrements the corresponding day of the month by one
[SHIFT] + [PAGEDOWN] - Decrements the corresponding month by one
[SHIFT] + [CTRL] + [PAGEDOWN] - Decrements the corresponding year by one
[PAGEUP] - Increments the corresponding day of the month by one
[SHIFT] + [PAGEUP] - Increments the corresponding month by one
[SHIFT] + [CTRL] + [PAGEUP] - Increments the corresponding year by one
Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

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

Thanks for the change.

I did not get the commit message body description, saying "The button should receive a correct focus outline when the 'tab' key is pressed." Could you be more specific, which button do you mean?

Also there are few comments you can find inline.

packages/main/src/DatePicker.js Outdated Show resolved Hide resolved
packages/main/src/DateRangePicker.js Outdated Show resolved Hide resolved
packages/main/src/DatePicker.js Show resolved Hide resolved
@unazko unazko requested a review from ilhan007 September 18, 2020 09:17
Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

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

Is the following expected: on PAGE_UP the startDate is changing, but on PAGE_DOWN both the start and end dates are changing.

In the video I start with PAGE_UP, that changes the start date and then I start pressing PAGE_DOWN that changes both
Screen Recording 2020-09-24 at 8.04.23 AM.zip

@unazko
Copy link
Contributor Author

unazko commented Sep 24, 2020

Is the following expected: on PAGE_UP the startDate is changing, but on PAGE_DOWN both the start and end dates are changing.

The behavior is broken after merging the conflicts and It has to be fixed.

 - The inner input value is now generated from formating local dates into
 a string.
 - Keyboard handling helper function now compares UTC dates.
@unazko unazko requested a review from ilhan007 September 24, 2020 09:57
@unazko
Copy link
Contributor Author

unazko commented Sep 24, 2020

The issue is now fixed.

@tsanislavgatev tsanislavgatev self-requested a review September 24, 2020 12:12
Changing the component input value via keyboard interactions is now
comfirmed by pressing enter keyboard key or focusing out of the input.
Tests are added, in order to cover the swapping of start and
end date of the range in the component input field, when start
date is after the end date.
@unazko
Copy link
Contributor Author

unazko commented Oct 1, 2020

According to the UX experts the behavior on Page Up and Page Down keyboard keys should change the value in the input field, but all validations have to be confirmed by pressing Enter keyboard key or by focusing out of the input field.
Therefore if the start date is after the end date of the range due to keyboard interaction, then the swapping between those values in the input field should happen when the range is confirmed.

@unazko unazko changed the title fix(ui5-daterange-picker): keyboard handling improvement feat(ui5-daterange-picker): keyboard handling improvement Oct 1, 2020
Copy link
Contributor

@tsanislavgatev tsanislavgatev left a comment

Choose a reason for hiding this comment

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

Also please merge master, as there is a fix about the min and max date of the DatePicker, and the bug is reproducible in your change. Also there are changes in the DateRangePicker file, which should be checked.

packages/main/src/DatePicker.js Outdated Show resolved Hide resolved
packages/main/src/DatePicker.js Outdated Show resolved Hide resolved
@unazko unazko requested a review from tsanislavgatev October 5, 2020 07:38
@tsanislavgatev
Copy link
Contributor

@ilhan007 Good on my side.

Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

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

Should not the component fire the "change" event, when the user changes the value with the newly added keyboard combinations, such as PAGEDOWN (applies for the rest as well).

Currently, in the DatePicker, DateRangePicker and DateTimePicker, the "change" event is not fired, when the value in the input is changed via these combinations and I was wondering if this is the way it should be. Even if you focus out the component after you change the date with PAGEDOWN for example, you won't get the "change" event.
Can you look into that?

@ilhan007
Copy link
Member

ilhan007 commented Oct 9, 2020

The DatePicker, DateRangePicker and DateTimePicker do not fire the "change" event on focus out, after you change the date with PAGEDOWN/UP. This will be handled in a separate issue.

@ilhan007 ilhan007 merged commit 84eb484 into SAP:master Oct 9, 2020
ilhan007 pushed a commit that referenced this pull request Oct 17, 2020
Depending on the caret symbol position, the corresponding date gets incremented
or decremented with one unit of measure by using the following keyboard
combinations:
[PAGEDOWN] - Decrements the corresponding day of the month by one
[SHIFT] + [PAGEDOWN] - Decrements the corresponding month by one
[SHIFT] + [CTRL] + [PAGEDOWN] - Decrements the corresponding year by one
[PAGEUP] - Increments the corresponding day of the month by one
[SHIFT] + [PAGEUP] - Increments the corresponding month by one
[SHIFT] + [CTRL] + [PAGEUP] - Increments the corresponding year by one

Fixes #1534
@@ -3,6 +3,7 @@ import Integer from "@ui5/webcomponents-base/dist/types/Integer.js";
import ValueState from "@ui5/webcomponents-base/dist/types/ValueState.js";
import CalendarDate from "@ui5/webcomponents-localization/dist/dates/CalendarDate.js";
import DateRangePickerTemplate from "./generated/templates/DateRangePickerTemplate.lit.js";
import RenderScheduler from "../../base/src/RenderScheduler.js";
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks several builds, please use the "@ui5/webcomponents-base/dist/RenderScheduler.js" form

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will for the future. Thanks.

ilhan007 pushed a commit that referenced this pull request Nov 11, 2020
Depending on the caret symbol position, the corresponding date gets incremented
or decremented with one unit of measure by using the following keyboard
combinations:
[PAGEDOWN] - Decrements the corresponding day of the month by one
[SHIFT] + [PAGEDOWN] - Decrements the corresponding month by one
[SHIFT] + [CTRL] + [PAGEDOWN] - Decrements the corresponding year by one
[PAGEUP] - Increments the corresponding day of the month by one
[SHIFT] + [PAGEUP] - Increments the corresponding month by one
[SHIFT] + [CTRL] + [PAGEUP] - Increments the corresponding year by one

Fixes #1534
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants