-
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 setting absolute dates without Enter key #7732
[EuiSuperDatePicker] Allow setting absolute dates without Enter key #7732
Conversation
CC @MichaelMarcialis in as well for thoughts (see above comment with bad mockup), since he's working on the next generation of EuiSuperDatePicker and I'd be curious to hear what UX he'd prefer for users manually entering dates for validation |
Thanks for the ping, @cee-chen! From a UX perspective, I agree with @sakurai-youhei that forcing the user to hit the enter key (or even click a separate button) to parse the date feels strange. I'm actually ashamed to admit it but, despite all my years here, I actually didn't know this was the behavior when manually typing in an absolute date. In That said, while I do agree with the changes, we are currently in the process of designing an updated and (hopefully) improved |
It's a relatively recent change since we started accepting more timestamp formats in that input. See #7331 (comment) for more context and back-and-forth we've already had around timestamp validation.
my 2c, for what it's worth: I don't love the idea of submission happening on "blur" instead of a deliberate choice to click a very explicit button and say "yes, this is definitely the date timestamp I want to try and parse". The UX on blur feels ambiguous - what if I gave up halfway or wanted to cancel my changes? There's literally no way to avoid doing so without causing an error state (or potentially parsing an incorrect date) the second I click away from the half-complete input. Michael, can you clarify what (if anything) you don't like about the explicit validation/submission button mock I made up above? It would only appear on user type/input change and if there are any changes to try and format/validate - it would not appear all the time.
I'd like us to figure this out now instead of later as this will help us with the upcoming new component. I'm looking for us to find a good compromise both in terms of UX and code complexity. |
@cee-chen @MichaelMarcialis Thank you for your comments. As a user, I would prefer as few clicks and types as possible, but I understand and totally agree that the code falls to the dark side. I had to increase the code complexity for the following minor perfection.
If it is reasonable to compromise on this, although the ambitious validation still remains, the code can be much simplified like THIS. The price of simplification is that when you leave the absolute tab with an invalid text input, the format error momentarily lights on red. I hope some improvement will happen in some timeframe if the pain points make sense to some amount of users. Otherwise (i.e., in case only I feel them), I will train myself to be more conscious of triggering the date validation. ;-) Thanks again for your review. I appreciate your time and attention to this PR. |
@MichaelMarcialis synced at our weekly team meeting about this and have agreed on an interim solution for now:
@sakurai-youhei I'll go ahead and take over this PR if no objections, since this a pretty large deviation from your proposed solution and I don't want to make you do the work twice! |
- using a `form` element allows us to take advantage of *both* native form enter key and click behavior OOTB without an extra listeners 🎉 - getting rid of the `Press enter` helptext also lets us simplify our logic and flex CSS 🎉 - fyi, slightly less granular commit history than my norm, apologies all
@cee-chen Thank you very much. I literally said, "Aha !!" when I played the new UI. I am amazed at how effectively the button catches my eye and encourages me to proceed naturally to the date parsing. I learned a lot - again :) - by seeing how the answer can be led out of the box. Back to my findings, here is one point from my perspective - horizontal grouping. The added button looks separate from the text field(, which may be done so intentionally, though), so I wonder how you find grouping it horizontally to the text field, like the pictures below. This is all from me. 👍
diff of codediff --git a/src/components/date_picker/super_date_picker/date_popover/absolute_tab.tsx b/src/components/date_picker/super_date_picker/date_popover/absolute_tab.tsx
index 2d6a5e356..3f1a2bac1 100644
--- a/src/components/date_picker/super_date_picker/date_popover/absolute_tab.tsx
+++ b/src/components/date_picker/super_date_picker/date_popover/absolute_tab.tsx
@@ -198,33 +198,35 @@ export class EuiAbsoluteTab extends Component<
: undefined
}
>
- <EuiFieldText
- compressed
- isInvalid={isTextInvalid}
- value={textInputValue}
- onChange={this.handleTextChange}
- onPaste={(event) => {
- this.parseUserDateInput(
- event.clipboardData.getData('text')
- );
- }}
- data-test-subj="superDatePickerAbsoluteDateInput"
- prepend={<EuiFormLabel>{labelPrefix}</EuiFormLabel>}
- />
+ <EuiFlexGroup responsive={false} gutterSize="s">
+ <EuiFieldText
+ compressed
+ isInvalid={isTextInvalid}
+ value={textInputValue}
+ onChange={this.handleTextChange}
+ onPaste={(event) => {
+ this.parseUserDateInput(
+ event.clipboardData.getData('text')
+ );
+ }}
+ data-test-subj="superDatePickerAbsoluteDateInput"
+ prepend={<EuiFormLabel>{labelPrefix}</EuiFormLabel>}
+ />
+ {hasUnparsedText && (
+ <EuiButtonIcon
+ type="submit"
+ className="euiSuperDatePicker__absoluteDateFormSubmit"
+ size="s"
+ display="base"
+ color={isTextInvalid ? 'danger' : 'primary'}
+ iconType="check"
+ aria-label={dateFormatButtonLabel}
+ title={dateFormatButtonLabel}
+ data-test-subj="parseAbsoluteDateFormat"
+ />
+ )}
+ </EuiFlexGroup>
</EuiFormRow>
- {hasUnparsedText && (
- <EuiButtonIcon
- type="submit"
- className="euiSuperDatePicker__absoluteDateFormSubmit"
- size="s"
- display="base"
- color={isTextInvalid ? 'danger' : 'primary'}
- iconType="check"
- aria-label={dateFormatButtonLabel}
- title={dateFormatButtonLabel}
- data-test-subj="parseAbsoluteDateFormat"
- />
- )}
</EuiFlexGroup>
)}
</EuiI18n> |
- have to use CSS trickery for this as the EuiFormRow component needs a refactor before it can take flex children :|
Nice catch, I think it makes sense to do! Sadly however we can't use an However, the good news is that we can still achieve the effect in your screenshots with some CSS hackery! 7253811 @MichaelMarcialis, do you mind giving this PR a final design review pass? If it looks good to you with no change requests, I'll go ahead and merge it. |
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.
Thanks for putting this together, @cee-chen! It looks like a good compromise. I left two small comments below, but nothing worth holding you up over. Assuming you're able to address them, I'll approve now.
src/components/date_picker/super_date_picker/date_popover/absolute_tab.tsx
Outdated
Show resolved
Hide resolved
helpText={ | ||
hasUnparsedText && !isTextInvalid | ||
? dateFormatError | ||
: 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.
Would it make sense to show the help text, regardless if the date is parsed or unparsed? It could be useful for users to see the accepted formats prior to making any changes.
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 think I have a slight preference to not showing it until needed? TBH, I do wonder how many people even use the absolute input, and it's kind of a lot of information to show 🙈
Co-authored-by: Michael Marcialis <michael.l.marcialis@gmail.com>
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
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.
Thanks again for your terrific improvements on this component Youhei, as always it's so incredibly appreciated!
Summary
This PR adds the following two implicit date parsing triggers to EuiSuperDatePicker's absolute tabs, which allow users to set absolute dates without hitting the Enter key.
componentWillUnmount()
.Steps to set 01:05:00.000 to 01:05:15.000:
Absolute
.01:00
.0
to replace by mouse.5
.Absolute
.00
to replace.15
.Update
Closes #7731
QA
Absolute
tab.Update
.Absolute
tab again.Refresh
without hitting the Enter key.Absolute
tab, even if the input date is not compliant with the date format, users shouldn't be notified about it.General checklist
Added documentationProps have proper autodocs (using@default
if default values are missing) and playground togglesChecked Code Sandbox works for any docs examplesIf applicable, added the breaking change issue label (and filled out the breaking change checklist)Updated the Figma library counterpart