-
Notifications
You must be signed in to change notification settings - Fork 841
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
[EuiSuperDatePicker] Allow the Absolute
tab to accept standardized ISO 8601, RFC 2822, and Unix timestamps
#7331
Conversation
…ndardized date formats
- per Moment's docs, the array API is significantly less performant, so let's avoid it unless there's no other option
cd7e5b4
to
b2b206f
Compare
const ALLOWED_USER_DATE_FORMATS = [ | ||
moment.ISO_8601, | ||
moment.RFC_2822, | ||
'X', // Unix timestamp in seconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also include 'x'
here to support integer millisecond-precision timestamps? I'd say they're as common as floating point timestamps, and supporting them both when using moment is really easy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried adding both, and unfortunately Moment doesn't seem to be able to handle it :( It errors on one or the other depending on the order in which you put them in the array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no, that's unfortunate :( Let's leave it as is then!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes look great! I found one edge case I think we should address before merging this in: when typing a timestamp, RFC8601 or RFC2822 formatted dates by hand, they're converted to a moment date instance immediately and usually end up being interpreted as unix timestamps sometime around January 1970 (which is expected because the input contains a single digit that's being interpreted as a correct timestamp). I'm not sure we can rely on the change event for this anymore if we want to allow users to type the value manually instead of always pasting it into the field
Ahh good catch! I do wonder how many people are typing into that input by hand 😅 For unix timestamps, I'm tempted to add some logic that checks for a minimum number of digits in a row first before attempting to parse it as a timestamp. I highly doubt anyone is going to be trying to convert a timestamp for 1970 EDIT: Also I assume absolutely no one is going to be typing in unix timestamps by hand, or that will fail too unless we add some kind of debounce to wait until users are done typing. Maybe that's just the safer option all around |
@cee-chen lol yeah, typing unix timestamps seems a bit extreme, but typing ISO8601 is very realistic. I think we could omit interpreting values as timestamps if they're shorter than 10 digits. |
I'm tempted to prefer the type debounce approach as it feels like it'll catch more edge cases around typing in general - e.g. for ISO 8601, the trailing |
- it's being passed around for no reason when it's not even being used
Alrighty, d8c760a adds a 1 second debounce to typing. Feel free to pull the branch down or wait til staging updates to re-test. I went back and forth on the 1s timing but thought that felt like the best compromise for slower typers and not feeling like it wasn't responding at all 🤔 |
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
@@ -84,26 +84,37 @@ export class EuiAbsoluteTab extends Component< | |||
}); | |||
}; | |||
|
|||
handleTextChange: ChangeEventHandler<HTMLInputElement> = (event) => { | |||
debouncedTypeTimeout: ReturnType<typeof setTimeout> | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a neat solution to the dual setTimeout
typing issue :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing the edge case! It looks and works great now 🎉
Thanks for the awesome review Tomasz!! ❤️ |
@cee-chen thanks, I'll look out for it. By the way, my mother probably types slower than 1 character per second sometimes! Would it be possible to make sure that the number is long enough to look like a relatively modern timestamp? For example ensuring that it has more than 8 digits would only prevent converting dates before 1973. I'm also concerned that a long debounce might prevent people from picking up on this feature. |
Sorry I just read your comment around the other case of users not having finished typing the timezone when it suddenly converts so I agree the debounce is needed. Although maybe also avoiding converting dates before 1973 might be good too. |
Those are some excellent points @mjaggard - after thinking about it a bit more, I think I lean away towards the need for a debounce at all and only attempting to parse dates on Enter keypress as opposed to on every input change. We can accomplish this well enough with text hints: This approach should accommodate both slow and fast typers as well as copy/paste use cases well enough with just a single keypress, and additionally has the advantage of telling users the expected formats ahead of time instead of after error. Will open a new PR here for that shortly. |
Sounds good. Can you also detect a paste event so that it's a single action to get a timestamp from a log file? |
Sure, why not! |
`v90.0.0`⏩`v91.0.0-backport.0`⚠️ While this upgrade pings many teams and has a large code diff, **the majority of the changes are snapshots or tests-related** and do not touch source code, so should theoretically only need a code review and not dedicated QA. The changes in EUI that required a large swathe of these updates are: - **EuiPopover** removed an extra unnecessary `<div>` wrapper on its anchors, which affected many snapshots and a few CSS overrides, which should have been updated - **EuiButtonGroup** now renders `<button>` elements instead of `<input type="radio">` elements for single selection, which affected both snapshots and E2E tests - **EuiSuperDatePicker**'s absolute date input now requires an `Enter` keypress when parsing dates (affected E2E tests) - **EuiComboBox**, when rendered with `singleSelection={{ plainText: 'true' }}`, no longer renders a pill (i.e. text). This combobox type now behaves more like an `EuiFieldText`, where the selection is rendered via input `value` instead. This affected a high amount of E2E tests (both FTR and Cypress), both in terms of updating assertions and changing selections, but should **not** significantly affect user experience - see elastic/eui#7332 for more. --- ## [`v91.0.0-backport.0`](https://github.com/elastic/eui/tree/v91.0.0-backport.0) **This is a backport release only intended for use by Kibana.** - Added `esqlVis`, `pipeBreaks`, and `pipeNoBreaks` icon glyphs. - `EuiSelectable` now allows configurable text truncation via `listProps.truncationProps` ([#7388](elastic/eui#7388)) - `EuiTextTruncate` now supports a new `calculationDelayMs` prop for working around font loading or layout shifting scenarios ([#7388](elastic/eui#7388)) **Bug fixes** - Fixed a bug with `EuiSelectable`s with custom `truncationProps`, where scrollbar widths were not being accounted for ([#7392](elastic/eui#7392)) ## [`91.0.0`](https://github.com/elastic/eui/tree/v91.0.0) - Updated the background color of `EuiPopover`s in dark mode to increase visibility & contrast against other page/panel backgrounds ([#7310](elastic/eui#7310)) - Memoized `EuiDataGrid` to prevent unneeded re-renders ([#7324](elastic/eui#7324)) - Added a configurable `role` prop to `EuiAccordion` ([#7326](elastic/eui#7326)) - Added a configurable `role` prop to `EuiGlobalToastList` ([#7328](elastic/eui#7328)) - For greater flexibility, `EuiSuperDatePicker` now allows users to paste ISO 8601, RFC 2822, and Unix timestamps in the `Absolute` tab input, in addition to timestamps in the `dateFormat` prop ([#7331](elastic/eui#7331)) - Plain text `EuiComboBox`es now behave more like a normal text field/input. Backspacing will no longer delete the entire value, and selected values can now be double clicked and copied. ([#7332](elastic/eui#7332)) - `EuiDataGrid`'s display settings popover now allows users to clear the "Lines per row" input before typing in a new number ([#7338](elastic/eui#7338)) - Improved the UX of `EuiSuperDatePicker`'s Absolute tab for users manually typing in timestamps ([#7341](elastic/eui#7341)) - Updated `EuiI18n`s with multiple `tokens` to accept dynamic `values` ([#7341](elastic/eui#7341)) **Bug fixes** - Fixed `EuiComboBox`'s `onSearchChange` callback to pass the correct `hasMatchingOptions` value ([#7334](elastic/eui#7334)) - Fixed an `EuiSelectableTemplateSitewide` bug where the `popoverButton` behavior would break if passed a non-DOM React wrapper ([#7339](elastic/eui#7339)) **Deprecations** - `EuiPopover`: deprecated `anchorClassName`. Use `className` instead ([#7311](elastic/eui#7311)) - `EuiPopover`: deprecated `buttonRef`. Use `popoverRef` instead ([#7311](elastic/eui#7311)) - `EuiPopover`: removed extra `.euiPopover__anchor` div wrapper. Target `.euiPopover` instead if necessary ([#7311](elastic/eui#7311)) - Deprecated `EuiButtonGroup`'s `name` prop. This can safely be removed. ([#7325](elastic/eui#7325)) **Breaking changes** - Removed deprecated `euiPaletteComplimentary` - use `euiPaletteComplementary` Instead ([#7333](elastic/eui#7333)) **Accessibility** - Updated `type="single"` `EuiButtonGroup`s to render standard buttons instead of radio buttons under the hood, per recent a11y recommendations ([#7325](elastic/eui#7325)) - `EuiAccordion` now defaults to a less screenreader-noisy `group` role instead of `region`. If your accordion contains significant enough content to be a document landmark role, you may re-configure it back to `region`. ([#7326](elastic/eui#7326)) - Reduced screen reader noisiness when sorting `EuiDataGrid` columns via toolbar ([#7327](elastic/eui#7327)) - `EuiGlobalToastList` now defaults to a `log` role. If your toasts will always require immediate user action, consider (with caution) using the `alert` role instead. ([#7328](elastic/eui#7328)) **CSS-in-JS conversions** - Updated `$euiFontFamily` and `$euiCodeFontFamily` to match Emotion fonts ([#7332](elastic/eui#7332)) --------- Co-authored-by: Cee Chen <constance.chen@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
Summary
closes elastic/kibana#159098 (will take a week or two to reach Kibana main, however)
This PR uses Moment's String + Formats API to attempt to parse non-
dateFormat
user input with several standardized timestamp formats as a fallback. I recommend reviewing by commit as I added a few misc cleanup items along the way.QA
Last 30 minutes
textAbsolute
tab2023-10-31T23:12:10+00:00
2023-10-31T23:12:10Z
Tue, 31 Oct 2023 23:12:10 +0000
1698793930
asdf
) or an invalid date (e.g.Jan 1st
) or an empty string, and confirm the input still flags these as invalidGeneral checklist
- [ ] Checked for accessibility including keyboard-only and screenreader modes- [ ] Checked in both light and dark modes- [ ] Checked in mobileand cypress tests- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)- [ ] Updated the Figma library counterpart