Skip to content
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

Add popover and prevent submit if duration params are invalid (#244) #291

Conversation

everett980
Copy link
Collaborator

@everett980 everett980 commented Dec 12, 2018

Which problem is this PR solving?

Short description of the changes

If the user times in an invalid duration, a popover is displayed to indicate that units are necessary. The submit button is disabled if any fields are invalid. To avoid jumping on the user as soon as the start typing, the popover is not visible until the user has blurred. After the user has blurred, the popover is visible as long as the field is invalid, even if the field becomes focused again.

244 invalid popover

@codecov
Copy link

codecov bot commented Dec 12, 2018

Codecov Report

Merging #291 into master will increase coverage by 0.13%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #291      +/-   ##
==========================================
+ Coverage   82.23%   82.36%   +0.13%     
==========================================
  Files         141      141              
  Lines        3112     3119       +7     
  Branches      647      651       +4     
==========================================
+ Hits         2559     2569      +10     
+ Misses        439      436       -3     
  Partials      114      114
Impacted Files Coverage Δ
.../components/SearchTracePage/SearchResults/index.js 75% <ø> (ø) ⬆️
...es/jaeger-ui/src/utils/redux-form-field-adapter.js 100% <100%> (+75%) ⬆️
...er-ui/src/components/SearchTracePage/SearchForm.js 87.12% <87.5%> (-0.38%) ⬇️
...neViewer/TimelineHeaderRow/TimelineViewingLayer.js 88.88% <0%> (+1.85%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1d258a...eb06e59. Read the comment docs.

@yurishkuro
Copy link
Member

I would rather display an error in the UI than try to infer which units the user had in mind by typing just a number. E.g. please specify units, e.g. "ms".

@tiffon
Copy link
Member

tiffon commented Dec 17, 2018

Per @yurishkuro comment, we can add a pattern attribute and use the title attribute to indicate a message.

pattern="\d[\d\.]*(us|ms|s|m|h)"
title="Enter a number followed by a duration unit, e.g. 20us, 30ms, 1.1s"

screen shot 2018-12-17 at 1 34 59 pm

The message only shows up if the user tries to submit the form while the field is invalid.

To take it a bit further, the :invalid pseudo-class can be used to style the input as red-ish.

screen shot 2018-12-17 at 1 36 31 pm

@@ -98,6 +99,11 @@ export function convertQueryParamsToFormDates({ start, end }) {
};
}

function addDefaultUnitToDuration(duration) {
if (!duration) return null;
return /[0-9]$/.test(duration) ? `${duration}ms` : duration;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: 1.1 would fail this regex.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just testing that if it ended in a number, append 'ms'. 1.1 ends in a number so it would pass. Regardless, I updated to your stronger pattern (but added a $ because 0ms0 would pass that pattern).

Signed-off-by: Everett Ross <reverett@uber.com>
@everett980 everett980 force-pushed the issue-244-add-default-unit-to-duration-query-params branch from a57d15d to b7616b9 Compare December 20, 2018 20:53
@ghost ghost assigned everett980 Dec 20, 2018
@ghost ghost added the review label Dec 20, 2018
Signed-off-by: Everett Ross <reverett@uber.com>
@everett980 everett980 changed the title Add default unit to duration search query params (#244) Add popover and prevent submit if duration params are invalid (#244) Dec 20, 2018
Signed-off-by: Everett Ross <reverett@uber.com>
Copy link
Member

@tiffon tiffon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The joys of redux-forms...

Changes look great. LMK if anything looks awry with my comments.

Signed-off-by: Everett Ross <reverett@uber.com>
Signed-off-by: Everett Ross <reverett@uber.com>
Copy link
Member

@tiffon tiffon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, one minor suggestion.

everett980 and others added 3 commits January 4, 2019 15:13
Signed-off-by: Everett Ross <reverett@uber.com>
Signed-off-by: Everett Ross <reverett@uber.com>
@everett980 everett980 merged commit ac4f7a7 into jaegertracing:master Jan 4, 2019
@ghost ghost removed the review label Jan 4, 2019
everett980 added a commit to everett980/jaeger-ui that referenced this pull request Jan 16, 2019
…tracing#244) (jaegertracing#291)

* Add validation for duration fields in SearchForm (jaegertracing#244)

Signed-off-by: Everett Ross <reverett@uber.com>

* Add tests for redux-form-field-adapter

Signed-off-by: Everett Ross <reverett@uber.com>

* Fix boolean prop type

Signed-off-by: Everett Ross <reverett@uber.com>

* Add boolean for input validation, change popover to show when inactive

Signed-off-by: Everett Ross <reverett@uber.com>

* Add tests for onChangeAdapter

Signed-off-by: Everett Ross <reverett@uber.com>

* Create separate ValidatedAdaptedInput for duration fields

Signed-off-by: Everett Ross <reverett@uber.com>

* Remove unnecessary curly braces

Signed-off-by: Everett Ross <reverett@uber.com>
Signed-off-by: Everett Ross <reverett@uber.com>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
…tracing#244) (jaegertracing#291)

* Add validation for duration fields in SearchForm (jaegertracing#244)

Signed-off-by: Everett Ross <reverett@uber.com>

* Add tests for redux-form-field-adapter

Signed-off-by: Everett Ross <reverett@uber.com>

* Fix boolean prop type

Signed-off-by: Everett Ross <reverett@uber.com>

* Add boolean for input validation, change popover to show when inactive

Signed-off-by: Everett Ross <reverett@uber.com>

* Add tests for onChangeAdapter

Signed-off-by: Everett Ross <reverett@uber.com>

* Create separate ValidatedAdaptedInput for duration fields

Signed-off-by: Everett Ross <reverett@uber.com>

* Remove unnecessary curly braces

Signed-off-by: Everett Ross <reverett@uber.com>

Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
…tracing#244) (jaegertracing#291)

* Add validation for duration fields in SearchForm (jaegertracing#244)

Signed-off-by: Everett Ross <reverett@uber.com>

* Add tests for redux-form-field-adapter

Signed-off-by: Everett Ross <reverett@uber.com>

* Fix boolean prop type

Signed-off-by: Everett Ross <reverett@uber.com>

* Add boolean for input validation, change popover to show when inactive

Signed-off-by: Everett Ross <reverett@uber.com>

* Add tests for onChangeAdapter

Signed-off-by: Everett Ross <reverett@uber.com>

* Create separate ValidatedAdaptedInput for duration fields

Signed-off-by: Everett Ross <reverett@uber.com>

* Remove unnecessary curly braces

Signed-off-by: Everett Ross <reverett@uber.com>
Signed-off-by: Everett Ross <reverett@uber.com>

Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants