Skip to content
This repository was archived by the owner on May 19, 2025. It is now read-only.

Conversation

@strada
Copy link
Contributor

@strada strada commented Jun 13, 2021

Types of changes

What types of changes does your code introduce?

  • Modify new range selection behavior when the start date has changed
  • Add some tests for various cases related to calcNewSelection method of DateRange component

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description

The user might want to move the start date without moving the end date, but the component is currently resetting the end date in all start date changes, which is not the ideal behavior in my opinion. See below a few gifs explaining the issue below:

How Google Analytics date selection behaves, (keeps end date selection):
date-range-behavior-ga-1
date-range-behavior-ga-2

How the current component behaves when the start date is changed:
date-range-behavior-rdr-1

After this PR:
date-range-behavior-rdr-pr

We might want to bump at least minor version as this changes the behavior, in order to improve functionality.

Related Issue: #464

@strada strada requested review from burakcan, kamyar and mkg0 as code owners June 13, 2021 15:37
@kamyar
Copy link
Contributor

kamyar commented Jun 13, 2021

Thanks for the PR @strada ! 🎉
Regarding the linked issue: Your Google Analytics example changed my mind.

What is your opinion regarding adding this behaviour behind a new config value similar to moveRangeOnFirstSelection?
Something like retainEndDateOnFirstSelection (We can move these two booleans to an enum in a major release later )

I see the benefit of the new behaviour but I will vote for having the old behaviour retained by default but supporting this simpler approach with the flag so that we avoid breaking people implementation.
Bumping the major is fine in my opinion but we are already planning a small cleanup that I personally would prefer to bump then and do a cleanup that makes your approach default.

Hiding the new behaviour behind a flag would help us get away with minor version bump + retain the old behaviour for people that prefer the resetting of endDate.

@burakcan @mkg0 @keremciu @onionhammer what do you folks think?

@strada strada requested a review from kamyar June 13, 2021 18:08
Copy link
Collaborator

@keremciu keremciu left a comment

Choose a reason for hiding this comment

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

LGTM! let's :shipit:

Copy link
Contributor

@kamyar kamyar left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@kamyar kamyar merged commit 17a7900 into hypeserver:master Jun 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants