-
Notifications
You must be signed in to change notification settings - Fork 18
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
MG-164 - Add input validation to chart modals #193
Conversation
b680d66
to
60e94c0
Compare
d73d954
to
af3e516
Compare
af3e516
to
f76267b
Compare
f76267b
to
8a64d3a
Compare
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.
Can we extract the pattern to a shared variable so we can easily change it? We will probably have more of the same check (for not-chart-related) things.
@@ -60,7 +64,7 @@ <h5 class="modal-title" id="alarmsTableModalLabel">Alarms Table</h5> | |||
<div class="mb-3"> | |||
<label for="update-interval" class="form-label">Update interval</label> | |||
<input | |||
type="text" | |||
type="number" |
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 interval shouldn't be a string something like 1h
or 10s
?
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.
Currently, we are only using intervals in seconds
for the first iteration. This will be improved in the next iteration to incorporate other intervals, minutes
, hours
, days
etc. This will also have to change based on the to
and from
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.
But it should still be string/text
@1998-felix
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.
Have select options sth like: 1s, 2s, 5s 10s, 20s, 30s,60s
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.
@1998-felix ^
Signed-off-by: felix.gateru <[email protected]>
Signed-off-by: felix.gateru <[email protected]>
Signed-off-by: felix.gateru <[email protected]>
Signed-off-by: felix.gateru <[email protected]>
0e1dd2d
to
31167c6
Compare
Signed-off-by: felix.gateru <[email protected]>
const invalidTimeFeedback = form.querySelector(".invalid-time"); | ||
invalidTimeFeedback.innerHTML = ""; | ||
const invalidTimeInput = form.querySelector("#stop-time"); | ||
invalidTimeInput.classList.remove("is-invalid"); |
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.
invalidTimeInput.classList.remove("is-invalid"); | |
form.querySelector(".invalid-time").innerHTML=""; | |
form.querySelector("#stop-time").classList.remove("is-invalid"); |
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.
This simplifies the code. Do this for the other files that do this.
Signed-off-by: felix.gateru <[email protected]>
bcd2b52
to
d9307f3
Compare
Signed-off-by: felix.gateru <[email protected]>
<option value="" disabled>Select an update interval</option> | ||
<option value="MAX">Maximum</option> | ||
<option value="MIN">Minimum</option> | ||
<option value="SUM">Sum</option> |
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.
missing count
aggregation. Check others too if they are missing any.
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.
This is done for bar charts and pie charts which I dont think will have the count aggregation as an option
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.
It will. say, I want a pie chart that shows the count of the various messages published by different devices in the same channel.
Signed-off-by: felix.gateru <[email protected]>
@@ -101,31 +105,46 @@ <h5 class="modal-title" id="lineChartModalLabel">Time Series Line Chart</h5> | |||
<label for="start-time" class="form-label">Start time</label> | |||
<input | |||
type="datetime-local" | |||
class="form-control mb-3" | |||
class="form-control mb-3 no-validate" |
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's with the no-validate
class and why is it just being used in the line chart and not the other time series charts?
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.
Should be removed.
Signed-off-by: felix.gateru <[email protected]>
What type of PR is this?
What does this do?
Adds input validation to the forms present in the create chart modals.
Which issue(s) does this PR fix/relate to?
Have you included tests for your changes?
Did you document any new/modified functionality?
Notes