Add date picker to silence form views by m-masataka · Pull Request #2262 · prometheus/alertmanager

m-masataka · 2020-05-18T18:14:50Z

Added date picker to silence form views to select date easier.
see #2226

Signed-off-by: m-masataka <m.mizukoshi.wakuwaku@gmail.com>

simonpasquier · 2020-05-19T12:41:47Z

Thanks for tackling this one! It seems that the CI isn't happy with your changes yet :)

Signed-off-by: m-masataka <m.mizukoshi.wakuwaku@gmail.com>

simonpasquier · 2020-05-25T15:03:11Z

This looks really good. I guess it would be good to support time too but probably it isn't that easy (at least the component supports only dates)?

roidelapluie · 2020-05-25T19:57:25Z

should we pick up a component that support both? can we reuse Prometheus' component?

w0rm · 2020-05-25T20:45:48Z

After a brief research I found this component: https://package.elm-lang.org/packages/mercurymedia/elm-datetime-picker/latest/

It supports both date and time, it also supports selecting the duration, which is what we need.

I didn't look in depth and what the integration would look like. @m-masataka I wonder if you have checked this one?

mizukoshi-m · 2020-05-29T15:51:11Z

Thank you for your comments.
@w0rm I didn't check that great component. I'll try to use it. thanks a lot!

Signed-off-by: m-masataka <m.mizukoshi.wakuwaku@gmail.com>

m-masataka · 2020-06-05T05:34:20Z

@w0rm Please give me some advice.
I tried Panaguitus/elm-datepicker because mercurymedia/elm-datetime-picker's time selector always reset 00:00 when open datetime picker( I think it is not useful for anyone ) .
However Panaguitus/elm-datepicker uses a component that has duplicated name with ui/app/src/DateTime.elm.
How can I avoid this component name duplication? ( or I have to use other component?)

here is the demo↓

w0rm · 2020-06-05T06:05:47Z

@m-masataka feel free to just move this module into Utils!

I also think we should get rid of the type alias and just use Posix instead of our DateTime type.

Signed-off-by: m-masataka <m.mizukoshi.wakuwaku@gmail.com>

m-masataka · 2020-06-14T15:33:40Z

I checked some datetime picker components, but I didn't find the perfect one.
Therefore I implement original datetime picker that based on mercurymedia/elm-datetime-picker.
Please check it out @w0rm.

w0rm · 2020-06-14T15:45:41Z

@m-masataka why did you have to inline the third party lib, can you please summarise what was not working in the original?

I also wonder whether there might be a copyright issue by inlining the code like this.

m-masataka · 2020-06-14T16:29:28Z

@w0rm the original one mostly work fine, it’s just a matter of preference.
I want to use date picker(not datetime picker) when there is no need to edit time. But the original time picker always reset 0:00 when date is picked.
In addtion, I don't like select field as time-picker much.

I also wonder whether there might be a copyright issue by inlining the code like this.

That is true for sure. I'll implement full scratch code as needed.
But in this PR, is it better if I only add mercurymedia/elm-datetime-picker?

w0rm · 2020-06-14T16:54:01Z

@m-masataka Oh, I see. Sorry, you actually wrote about this before!

Panaguitus/elm-datepicker didn't work either? I think renaming or getting rid of our internal types is totally fine if that is needed.

m-masataka · 2020-06-14T17:59:15Z

Panaguitus/elm-datepicker is perfect for me expect unnecessary view component such as today button and date limit.
Panaguitus/elm-datepicker strongly depends on other datetime component such as elm-datetime. I'm concerned about that datetime component will conflict with other packages in the future.

m-masataka · 2020-06-14T18:13:13Z

I think renaming or getting rid of our internal types is totally fine if that is needed.

Ah sorry, we can avoid any conflict.
Which do you think is better we should use, single picker or duration picker? @w0rm

w0rm · 2020-06-14T18:38:19Z

@m-masataka I think picking a duration is better, this way a user won't have to open the dialog twice for each date time.

This looks pretty good, just the colours need to be tweaked to match what we have so far.

@m-masataka I also don't mind if we implement our own datepicker inside alertmanager codebase from scratch. I am just a bit worried that we shouldn't copy paste somebody else's code and sign it off as our own.

m-masataka · 2020-06-15T10:17:03Z

Thank you @w0rm.
Unfortunately, I have one bad news about Panaguitus/elm-datepicker.
This duration picker can't pick same date as start and end. (here is the code.)
I think this feature is not user-friendly. What's your opinion on this?

We have some choice

Use Panaguitus/elm-datepicker's duration picker
Use Panaguitus/elm-datepicker's datetime picker ( not use duration picker)
Use mercurymedia/elm-datetime-picker's duration picker
Implement datepicker inside alertmanager with full scratch code

I think 3 (or 4) is better than others.

w0rm · 2020-06-15T20:03:43Z

@m-masataka I fully agree with you that 3 or 4 is better. Will you be willing to contribute a custom date picker (option 4)? Long term I would prefer this option, because it is easier to migrate our own code if a new version of Elm comes up.

Signed-off-by: m-masataka <m.mizukoshi.wakuwaku@gmail.com>

m-masataka · 2020-07-15T12:06:34Z

Hi @w0rm
I Implemented DateTimePicker inside alertmanager (and this is not third party libs).
Please review this.

roidelapluie · 2020-07-15T13:53:58Z

cc @juliusv

juliusv · 2020-07-15T14:07:24Z

Looks pretty IMO! I can't review Elm stuff though, I have never touched it :( @w0rm or @simonpasquier?

w0rm · 2020-07-15T18:45:25Z

+
+
+type alias PickerConfig msg =
+    { zone : Zone


I think it is safe to assume that this is always going to be in utc. You can simplify a lot of code and just use utc everywhere.

w0rm · 2020-07-15T18:49:01Z

+                                    [ text "x" ]
+                                ]
+                            , div [ class "modal-body" ]
+                                [ viewDateTimePicker dateTimePickerConfig form.dateTimePicker ]


If we change to use utc everywhere, then viewDateTimePicker could take UpdateDateTimePicker and the dateTimePickerConfig would not be needed.

Thank's. I removed ...Config.

w0rm · 2020-07-15T18:52:25Z

 fromSilence { id, createdBy, comment, startsAt, endsAt, matchers } =
+    let
+        startsPosix =
+            case Utils.Date.timeFromString (DateTime.toString startsAt) of


You can use Result.toMaybe

w0rm · 2020-07-15T18:52:51Z

+                    Nothing
+
+        endsPosix =
+            case Utils.Date.timeFromString (DateTime.toString endsAt) of


Use Result.toMaybe

w0rm · 2020-07-15T18:55:28Z

+                            form.pickedStart
+
+                endsAtTime =
+                    case timeFromString form.endsAt.value of


Pipe to Result.toMaybe

w0rm · 2020-07-15T18:58:51Z

@m-masataka looks great, thank you for implementing this non trivial feature!

I left a few comments that may simplify things a bit. But I am fine if this goes as is.

Signed-off-by: m-masataka <m.mizukoshi.wakuwaku@gmail.com>

simonpasquier · 2020-07-24T12:52:24Z

Thanks a lot @m-masataka for the hard work! I'm not really qualified to review the Elm code but I'll trust @w0rm :)

m-masataka added 2 commits May 19, 2020 03:01

add datepicker

2d78828

Signed-off-by: m-masataka <m.mizukoshi.wakuwaku@gmail.com>

fix import error

40f4462

Signed-off-by: m-masataka <m.mizukoshi.wakuwaku@gmail.com>

m-masataka added 2 commits May 25, 2020 11:14

fix unnecessary import from DateTime package

533e743

Signed-off-by: m-masataka <m.mizukoshi.wakuwaku@gmail.com>

fix unnecessary import utc

5079a3e

Signed-off-by: m-masataka <m.mizukoshi.wakuwaku@gmail.com>

simonpasquier requested a review from w0rm May 25, 2020 15:01

change datetime picker component

97972ce

Signed-off-by: m-masataka <m.mizukoshi.wakuwaku@gmail.com>

added datetime picker utils

01f7586

Signed-off-by: m-masataka <m.mizukoshi.wakuwaku@gmail.com>

added datetime picker utils

01f36cd

Signed-off-by: m-masataka <m.mizukoshi.wakuwaku@gmail.com>

w0rm reviewed Jul 15, 2020

View reviewed changes

w0rm approved these changes Jul 15, 2020

View reviewed changes

m-masataka added 2 commits July 18, 2020 02:17

remove config

29d775d

Signed-off-by: m-masataka <m.mizukoshi.wakuwaku@gmail.com>

replace case expressions to Result.toMaybe

4e5281a

Signed-off-by: m-masataka <m.mizukoshi.wakuwaku@gmail.com>

simonpasquier merged commit 73db787 into prometheus:master Jul 24, 2020

This was referenced Aug 14, 2020

date/time picker for create silences page #1093

Closed

date/time selector #2226

Closed

Conversation

m-masataka commented May 18, 2020

Uh oh!

simonpasquier commented May 19, 2020

Uh oh!

simonpasquier commented May 25, 2020

Uh oh!

roidelapluie commented May 25, 2020

Uh oh!

w0rm commented May 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mizukoshi-m commented May 29, 2020

Uh oh!

m-masataka commented Jun 5, 2020

Uh oh!

w0rm commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m-masataka commented Jun 14, 2020

Uh oh!

w0rm commented Jun 14, 2020

Uh oh!

m-masataka commented Jun 14, 2020

Uh oh!

w0rm commented Jun 14, 2020

Uh oh!

m-masataka commented Jun 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m-masataka commented Jun 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

w0rm commented Jun 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m-masataka commented Jun 15, 2020

Uh oh!

w0rm commented Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m-masataka commented Jul 15, 2020

Uh oh!

roidelapluie commented Jul 15, 2020

Uh oh!

juliusv commented Jul 15, 2020

Uh oh!

w0rm Jul 15, 2020

Choose a reason for hiding this comment

Uh oh!

w0rm Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m-masataka Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

w0rm Jul 15, 2020

Choose a reason for hiding this comment

Uh oh!

w0rm Jul 15, 2020

Choose a reason for hiding this comment

Uh oh!

w0rm Jul 15, 2020

Choose a reason for hiding this comment

Uh oh!

w0rm commented Jul 15, 2020

Uh oh!

simonpasquier commented Jul 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

w0rm commented May 25, 2020 •

edited

Loading

w0rm commented Jun 5, 2020 •

edited

Loading

m-masataka commented Jun 14, 2020 •

edited

Loading

m-masataka commented Jun 14, 2020 •

edited

Loading

w0rm commented Jun 14, 2020 •

edited

Loading

w0rm commented Jun 15, 2020 •

edited

Loading

w0rm Jul 15, 2020 •

edited

Loading