chore: Enhancing date-range-picker pages #3107
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3107 +/- ##
=======================================
Coverage 96.37% 96.37%
=======================================
Files 783 783
Lines 22039 22039
Branches 7553 7553
=======================================
Hits 21241 21241
Misses 791 791
Partials 7 7 ☔ View full report in Codecov by Sentry. |
dpitcock
force-pushed
the
denpitco-date-range-picker-page-enhancements
branch
2 times, most recently
from
b5eb0ae to
5afccb7
Compare
dpitcock
force-pushed
the
denpitco-date-range-picker-page-enhancements
branch
from
0cb9036 to
08194ce
Compare
| <Link id="focus-dismiss-helper">Focusable element before the date range picker</Link> | ||
| <br /> | ||
| <br /> | ||
| <ScreenshotArea> |
There was a problem hiding this comment.
If there is a <ScreenshotArea>, it is likely used in some screenshot tests. Did you clean them up before merging this?
There was a problem hiding this comment.
All tests that run against <...>/index.html#/light/date-range-picker/with-default-date and <...>/index.html#/light/date-range-picker/with-default-date-only will continue to run but this time against the same page<...>/index.html#/light/date-range-picker/with-value with params added to load the page with the component as date-only or not?dateOnly=true and ?dateOnly=false. So the same screenshots will exist. However as this PR is merged, the regression tests will see the former as removed, and those after it as new, even though they are visually the same
| const monthOnly = false; | ||
| const absoluteFormat = | ||
| urlParams.absoluteFormat ?? (dateRangePickerDemoDefaults.absoluteFormat as DateRangePickerProps.AbsoluteFormat); | ||
| const showRelativeOptions = urlParams.showRelativeOptions ?? dateRangePickerDemoDefaults.showRelativeOptions; | ||
| const dateOnly = urlParams.dateOnly ?? dateRangePickerDemoDefaults.dateOnly; | ||
| const disabledDates = | ||
| (urlParams.disabledDates as DisabledDate) ?? (dateRangePickerDemoDefaults.disabledDates as DisabledDate); | ||
| const withDisabledReason = urlParams.withDisabledReason ?? dateRangePickerDemoDefaults.withDisabledReason; | ||
| const invalid = urlParams.invalid ?? dateRangePickerDemoDefaults.invalid; | ||
| const warning = urlParams.warning ?? dateRangePickerDemoDefaults.warning; | ||
| const rangeSelectorMode = | ||
| urlParams.rangeSelectorMode ?? | ||
| (dateRangePickerDemoDefaults.rangeSelectorMode as DateRangePickerProps.RangeSelectorMode); |
There was a problem hiding this comment.
Wouldn't it be easier to make all defaults false/undefined, instead of writing this wall of text for every page?
There was a problem hiding this comment.
It's a tradeoff. I understand your approach. While it may seem verbose, using centralized defaults in dateRangePickerDemoDefaults allows us to maintain consistency across multiple pages and easily update default values in one place. This approach reduces the risk of inconsistencies and makes maintenance easier in the long run. It's worth noting that specific initial values can be assigned in place of the defaults if desired for any particular page. Also, not all pages have this 'wall of text,' as many don't allow for changes to as many props, resulting in a more concise implementation on those pages.
There was a problem hiding this comment.
Other integ tests screenshots added in CR-165057689
dpitcock
force-pushed
the
denpitco-date-range-picker-page-enhancements
branch
from
088efae to
d668377
Compare
| <Link id="focus-dismiss-helper">Focusable element before the date range picker</Link> | ||
| <br /> | ||
| <br /> | ||
| <ScreenshotArea> |
There was a problem hiding this comment.
This is the ScreenshotArea that will be used to continue testing for both default-date and default-date-only tests when the url with the right params are passed into this page
Description
This adds more controls to the date-range-picker pages so we can toggle on and off features. It also accepts urlParams to toggle the features too so specific functionality can be shared.
The updated pages will make it easier to add additional changes for when there is a granularity prop added to the date-range-picker component
Related links, issue #, if available: n/a
How has this been tested?
Integ tests updated to reflect new urls and utilize params accordingly
Updates to internal Integ Test made to match this package in CR-165057689
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.