Skip to content

feat: refactoring and new features for pickers #2598

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

Merged
merged 127 commits into from
Jan 8, 2021
Merged

Conversation

vladitasev
Copy link
Contributor

@vladitasev vladitasev commented Dec 18, 2020

Date/time components refactoring, bugfixes and updates

Summary:

Main takeaways from the refactoring (Do's and don'ts):

❌ Get references to child components with this.shadowRoot.querySelector and change their properties
✔️ Create new private properties, bind them to the child components in .hbs and change these properties instead

❌ Change invalid application-supplied properties to valid ones (f.e. value)
✔️ Never change what was set by the application, create "effective" getters f.e. get _effectiveValue and use them instead

❌ Create objects in the constructors for properties/events of child components and use them to bind (f.e. _calendarInfo)
✔️ Create separate properties and functions (f.e. onCalendarChange, _calendarTimestamp, _calendarSelectedDates)

❌ Do heavy work such as recalculate computed state or change other components in event handlers
✔️ Only change the component's own state in event handlers and let invalidation occur and computed state be recalculated

❌ Synchronize properties whenever one of them changes (f.e. having two properties with the same value in different formats)
✔️ Instead keep just one property (source of truth) and create a getter instead of the second one

❌ Let component private/public properties bound to composed component's properties get out of sync
✔️ Always bind to the change (and similar) events of child components and update all bound properties in your component

❌ Have heavy computation (f.e. loopDate < this._minDate) in loops (degrades IE performance tremendously)
✔️ Move these before the loop (f.e. const minDate = this._minDate) and compare to the extracted value

❌ Copy/paste code for similar functionality
✔️ Use one of the following approaches: inheritance (f.e. CalendarPart.js), composition(f.e. ui5-time-selection) or library (modules for common code such as modifyValueBy.js, ExtremeDates.js)

❌ Create tests that rely on the state of the test page from the previous test (f.e. some value being set in a date picker)
✔️ Only test components that were never modified or reload the page (browser.url(...)) first before testing the same ones

❌ Use setValue, setProperty or setAttribute to simulate user interaction (as opposed to initial state) in tests
✔️ Only use what the user can actually do: keys and click because these have different logic in the components compared to setting values (f.e. user interaction fires events while the app setting values directly does not).

New Components

  • ui5-time-selection (private component): the counterpart of ui5-calendar for time selection. This component wraps the 4 wheel sliders.

New Features

  • Implemented the Click+Shift and Enter+Shift keyboard combinations. Now it is possible to select/deselect a range of dates in selection="Multiple" mode with Shift (instead of clicking them one by one).
  • Implemented the Space+Shift keyboard combination. Now it is possible to select/deselect the whole week in selection="Multiple" mode.
  • Implemented keyboard handling for ui5-calendar when selection="Range". When the user changes the focus with the keyboard, it has the same effect as hovering with the mouse.
  • Implemented aria-selected for day/month/year picker.
  • ui5-calendar now has a pre-determined fixed size and no longer adjusts itself upon switching pickers. ui5-calendar now has a padding.
  • ui5-daypicker now always shows 42 days (6 weeks) regardless of any conditions (0001 year, 9999 year, etc...).

Bugfixes

  • Documentation is now properly built for components with several levels of base classes. Additionally, public methods are shown for base classes. Protected entities are no longer shown.
  • ui5-wheelslider: the component is now usable on all themes (it was impossible to navigate the component on themes other than fiori3)
  • ui5-datepicker: when the user is typing (and normalizeValue) is called, the user gets the correct date regardless of timezone (used to produce the previous days on negative timezones).
  • All components with ItemNavigation: pressing PageUp/PageDown no longer breaks item navigation. Before the change, any component that does not use mode=Paging (all components but the pickers) got stuck if the user tried to use the page keys.
  • Clicking the "<" arrow in ui5-calendar no longer breaks the keyboard navigation inside the day picker
  • Tab and Shift+Tab now work correctly for pickers in both directions (Year and Month buttons are not skipped with Shift+Tab).
  • ui5-calendar keyboard handling works on IE again
  • ui5-calendar: clicking the "<" and ">" arrows no longer focuses the first day of the month, but focus is preserved.
  • ui5-yearpicker: keyboard navigation does not get confused around year 0001.
  • ui5-monthpicker: when closing and re-opening the monthpicker the selected month is not lost
  • ui5-monthpicker: when dates among multiple months are selected in ui5-calendar with selection=Multiple all of these months are marked as selected in the months view.
  • ui5-calendar: F4 and Shift+F4 only have effect when day picker is the current picker, as in the accessibility spec. When the monthpicker is open and you press Shift+F4, it should not switch to the year picker.
  • ui5-daypicker: added i18n text for "Today" which used to be hard-coded in English
  • ui5-daypicker: fixed the broken design around the dates Jan 1, 0001 and Dec 31 9999.
  • ui5-daterange-picker: changing the selected dates with keyboard shortcuts over the input correctly fires a change event.
  • ui5-daterange-picker: the component works if an empty delimiter is provided ("-" is used).
  • ui5-datetime-picker: changing the date with the keyboard when the input is focused (PageUp/etc...) no longer loses the hours, minutes and seconds.
  • ui5-datetime-picker: pressing PageDown/PageUp etc... on an empty input no longer produces a JavaScript error.
  • ui5-datetime-picker: the Submit button is now disabled if no date is selected (and the currently focused date will not be auto-selected if you press Submit)
  • ui5-timepicker: it is now possible to have an empty value (before the value was automatically set to the current time if empty during onBeforeRendering)

Fundamental refactoring

  • Removed all code that queries the DOM or changes it directly. Only use state.
  • Removed all code that gets instances to child components (day/month/year picker) and sets their state. Only use the .hbs for passing state from parents (calendar) to children (pickers) or call their public methods (but don't set their state directly).
  • Moved all code specific to day/month/year picker away from calendar to the respective components. Cut some duplicate code too.
  • Calendar no longer calculates and passes the month/year texts for the calendar header. Now it instead passes the timestamp and primaryCalendarType (both normalized) and the calendar header calculates its texts itself.
  • The communication flow of ui5-calendar, ui5-daypicker, ui5-monthpicker and ui5-yearpicker is simplified. Calendar changes the pickers with bindings in the .hbs only and listens to their events to update its state when they change. The 3 pickers are completely synchronized now via the calendar for all possible common properties.
  • Imperative-style code removed and replaced with code that only changes the state. This includes, but is not limited, to code that was previously entirely DOM-based (tabindex handling, range selection hover handling).
  • Removed ItemNavigation from ui5-daypicker, ui5-monthpicker and ui5-yearpicker. This has the following benefits: single source of truth for the selected item (this.timestamp instead of this.timestamp and this._itemNav.selectedIndex), no more events and waiting for the renderer to finish, no more double logic for keyboard handling.
  • Changed the way focus works for pickers.
  • Moved all minDate/maxDate restrictions to one place.
  • Completely changed Tab and Shift+Tab logic. There is no more focus manipulation. Instead, the components are rendered in the preferred order in DOM so that focusing can work naturally, and only the header is positioned absolutely above the picker.

ItemNavigation changes:

  • Paging mode removed, pageSize property removed, support for PageUp/PageDown keys removed, afterFocus and borderReach events removed from item navigation. This mode was added specifically for the purpose of the pickers (day/month/year) but it added more complexity and created more problems than it resolved. It required 2 callbacks (hasNextPage, hasPreviousPage) to be implemented and 2 events to be listened for (borderReach and afterFocus) for it to work. Additionally, it required the pickers to have re-rendered before it can continue working on the newly loaded content, which required extra synchronization.
  • The bigger conceptual problem (apart from the technical complications) is that the core business logic of the pickers (day/month/year) such as determining the currently focused date, was done by querying the current index of the ItemNavigation. This is detrimental to the pickers' code quality because the this.timestamp property should be the source of truth for that, but there's nothing to synchronize this property back when the user changed the focused date with the keyboard. Therefore this property was misused (used only for input) and the item navigation current index was also misused (for determining the component's state). After this change, the keyboard navigation is done internally by these components, but the big difference is that this.timestamp is used as the source of truth (the role that the item navigation current index had) for all calculations. And when the timestamp changes, this by definition updates the item navigation-related state.

Framework-level changes:

  • added compareValues metadata entity for properties of type multiple: true. It allows to consider arrays equal by value (not only by reference). This is a big performance optimization for IE, otherwise does not affect logic at all.
  • ScrollEnablement.js: enhanced the scrollTo method to be able to retry an arbitrary number of times until it's really able to scroll the container.

IE11-related

  • Added the Array.prototype.findIndex polyfill which was used in several places already (most notably date-time components) and broke the keyboard navigation and other functionality on IE.
  • Updated the Element.prototype.closest polyfill to be shadow-DOM friendly
  • Greatly improved the performance on IE (about 10 times) by not calling the expensive getters (_calendarDate, _minDate, _maxDate) in loops but caching them upfront.

Tests

  • Deleted some tests that were exact duplicates of each other, or verified that a property that was set (timestamp) is the same. This tests the framework, not the business logic. All such tests were in Calendar.spec.js.
  • Reset most of the calendar tests (to reload the page) because they depended on each other.

New code structure

  • CalendarPart.js: base class for Calendar.js, DayPicker.js, MonthPicker.js and YearPicker.js. Manages all metadata and code common for all calendar parts (most notably timestamp and all timestamp and calendar date related transformations).
  • DateComponentBase.js: base class for DatePicker.js and CalendarPart.js. Manages all metadata and code common for DatePicker and the calendar parts (most notably min/max date, format and calendar type-related properties and logic).
  • ui5-time-picker and ui5-duration-picker now share the same codebase (TimePickerBase.js base class with 95% of the common code) and they only override a tiny portion of it.
  • ui5-time-picker, ui5-datetime-picker and ui5-duration-picker now use internally the new ui5-time-selection component that handles logic related to the 3 sliders.

image
image
image

BREAKING CHANGES:

  • The selection property of ui5-calendar and ui5-daypicker has been renamed to selectionMode. Its respective type is now called CalendarSelectionMode instead of CalendarSelection and is now part of the @ui5/webcomponents project (used to be in @ui5/webcomponents-base). The values and usage are the same.
  • The @ui5/webcomponents-base/dist/types/CalendarSelection.js type is now removed. Use the @ui5/webcomponents/dist/types/CalendarSelectionMode.js type (has the same values).
  • The ui5-time-picker can now have an empty value, as required by specifications, aligning the component with ui5-datepicker, ui5-datetime-picker and ui5-daterange-picker in this regard. Before the component would automatically set the current time, if the value property was empty. Now the current time will not be set unless the user presses the "OK" button. If you want to preserve the existing behavior, you should explicitly initialize the time picker with the current time.
  • The ItemNavigation paging mode (ItemNavigationBehavior.Paging) was removed. This should not affect any public code as this mode was strictly used for the day/month/year pickers.

closes: #2581
closes: #2598
closes: #2627
closes: #2595

@vladitasev vladitasev merged commit 3e684b4 into master Jan 8, 2021
@vladitasev vladitasev deleted the refactor-calendar branch January 8, 2021 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants