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): initial implementation #1785

Merged
merged 11 commits into from
Jun 30, 2020

Conversation

tsanislavgatev
Copy link
Contributor

There is a commented method which will be deleted before merge.
Also test will be added, the PullRequest is for the functional code.

Copy link
Contributor

@fifoosid fifoosid left a comment

Choose a reason for hiding this comment

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

While testing I noticed the following bugs:

  • There is no pointer cursor when hovering the icon for opening
  • There is no hovering effect on the icon as well
  • When the daterange picker is opened, clicking outside of the popover doesn't close the picker
  • On the test page, on the fourth picker(DateRangePicker with minDate 01/09/2019 and maxDate 01/11/2019) user is not able to go to the next month via the arrow for the next month in the upper right corner.

packages/main/src/DateRangePicker.js Outdated Show resolved Hide resolved
packages/main/src/DateRangePicker.js Outdated Show resolved Hide resolved
packages/main/src/DateRangePicker.js Outdated Show resolved Hide resolved
packages/main/src/DateRangePicker.js Show resolved Hide resolved
packages/main/src/DateRangePicker.js Outdated Show resolved Hide resolved
packages/main/test/samples/DateRangePicker.sample.html Outdated Show resolved Hide resolved
packages/main/test/samples/DateRangePicker.sample.html Outdated Show resolved Hide resolved
packages/main/test/samples/DateRangePicker.sample.html Outdated Show resolved Hide resolved
packages/main/test/samples/DateRangePicker.sample.html Outdated Show resolved Hide resolved
packages/main/src/DateRangePicker.hbs Outdated Show resolved Hide resolved
packages/main/src/DateRangePicker.js Outdated Show resolved Hide resolved
packages/main/src/DateRangePicker.js Outdated Show resolved Hide resolved
packages/main/src/DateRangePicker.js Show resolved Hide resolved
@ilhan007 ilhan007 changed the title feat(ui5-daterangepicker): initial implementation feat(ui5-daterange-picker): initial implementation Jun 15, 2020
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.

Try to avoid the duplication of those 3 relatively large methods by introducing a new method with different implementation in DatePicker and DateRangePicker. I left the details inline.

packages/main/src/DateRangePicker.js Outdated Show resolved Hide resolved
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.

I think it is good to add bigger min-width to the DateRangePicker so the data range is visible, not truncated as currently. For example we increased the min-width DateTimePicker for the same reason as it shows date and time, taking more space.

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.

The change event is fired on clicking the first date, then on clicking the second date. Ensure it is fired on the second date only and cover the functionality with a test.

@ilhan007 ilhan007 dismissed fifoosid’s stale review June 23, 2020 14:08

comments are resolved

@ilhan007 ilhan007 merged commit 4c11286 into master Jun 30, 2020
@ilhan007 ilhan007 deleted the daterangepicker-implementation branch June 30, 2020 12:47
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.

3 participants