refactor(TimezoneSelector): simplify override logics and tests #19090
Conversation
ktmud
force-pushed
the
refactor-timezone-selector
branch
2 times, most recently
from
0987b3a
to
a2095c8
Compare
| moment.tz(currentDate, a.timezoneName).utcOffset() - | ||
| moment.tz(currentDate, b.timezoneName).utcOffset(); | ||
|
|
||
| TIMEZONE_OPTIONS.sort(TIMEZONE_OPTIONS_SORT_COMPARATOR); |
There was a problem hiding this comment.
#19085 requires select options to be pre-sorted.
| // others are ranked by offset | ||
| expect(options[1]).toHaveTextContent('GMT -11:00 (Pacific/Pago_Pago)'); | ||
| expect(options[2]).toHaveTextContent('GMT -10:00 (Hawaii Standard Time)'); | ||
| expect(options[3]).toHaveTextContent('GMT -10:00 (America/Adak)'); |
There was a problem hiding this comment.
New tests for option ordering.
| expect(selectOption).toBeInTheDocument(); | ||
| userEvent.click(selectOption); | ||
| expect(onTimezoneChange).toHaveBeenCalledTimes(1); | ||
| expect(onTimezoneChange).toHaveBeenLastCalledWith('America/Cambridge_Bay'); |
There was a problem hiding this comment.
Test the updated timezone value after selecting an option
|
|
||
| return ( | ||
| <Select | ||
| ariaLabel={t('Timezone selector')} | ||
| css={{ minWidth: MIN_SELECT_WIDTH }} // smallest size for current values | ||
| onChange={onTimezoneChange} | ||
| value={timezone || DEFAULT_TIMEZONE.value} |
There was a problem hiding this comment.
Make sure value is a valid timezone. timezone may be different than updatedTz.
| [onTimezoneChange], | ||
| const validTimezone = useMemo( | ||
| () => matchTimezoneToOptions(timezone || moment.tz.guess()), | ||
| [timezone], |
There was a problem hiding this comment.
Changed from useRef to simply triggering a setState in useEffect.
ktmud
force-pushed
the
refactor-timezone-selector
branch
from
a2095c8
to
4f7a93c
Compare
Codecov Report
@@ Coverage Diff @@
## master #19090 +/- ##
==========================================
- Coverage 66.48% 66.48% -0.01%
==========================================
Files 1644 1644
Lines 63492 63491 -1
Branches 6459 6457 -2
==========================================
- Hits 42212 42211 -1
Misses 19610 19610
Partials 1670 1670
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
/testenv up |
|
@eschutho Ephemeral environment spinning up at http://35.86.91.68:8080. Credentials are |
|
/testenv up FEATURE_ALERT_REPORTS=True |
|
@eschutho Ephemeral environment spinning up at http://35.163.121.68:8080. Credentials are |
|
Ephemeral environment shutdown and build artifacts deleted. |
(cherry picked from commit 3a78165)
mistercrunch
added
🍒 1.5.0
🍒 1.5.1
🍒 1.5.2
🍒 1.5.3
🏷️ bot
SUMMARY
Encountered a broken unit test in
TimezoneSelectorafter making changes in #19085 , found some room for improvements so I refactored this component a little: simplified the timezone overriding logics and made the unit tests more complete. See comments for details.As the changes are quite significant and the component itself is small, it may be easier to review the diff in split view.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No visual changes
TESTING INSTRUCTIONS
Test places where timezone selector is used. The functionality should not change.
ADDITIONAL INFORMATION