Upgrade Alertmanager UI to Elm 0.19 #1539
Conversation
Signed-off-by: Andrey Kuzmin <andrey.kuzmin@soundcloud.com>
Signed-off-by: Andrey Kuzmin <andrey.kuzmin@soundcloud.com>
Signed-off-by: Andrey Kuzmin <andrey.kuzmin@soundcloud.com>
Signed-off-by: Andrey Kuzmin <andrey.kuzmin@soundcloud.com>
Signed-off-by: Andrey Kuzmin <andrey.kuzmin@soundcloud.com>
|
@mxinden sure, take your time! It was relatively straightforward, mostly mechanical work, except maybe for the changes in the |
|
Thanks! Will start using this and see if I notice anything |
ui/app/Makefile
Outdated
| @$(DOCKER_CMD) elm make src/Main.elm --yes --output $(TEMPFILE_JS) | ||
| @$(DOCKER_CMD) uglifyjs $(TEMPFILE_JS) --compress unused --mangle --output $(@) | ||
| @$(DOCKER_CMD) rm -rf elm-stuff | ||
| @$(DOCKER_CMD) elm make src/Main.elm --output $(TEMPFILE_JS) |
There was a problem hiding this comment.
we can add the --optimize flag, right?
There was a problem hiding this comment.
108594 bytes on script.js down to 93329 bytes it seems
There was a problem hiding this comment.
Totally missed this, thanks for noticing!
There was a problem hiding this comment.
Thanks for this huge work. I hope elm-upgrade did most of it.
With #1352 merged, I would suggest I cut an alpha release soon. I would like this to be included. @stuartnelson3 what do you think?
|
|
||
| script.js: elm-env format $(ELM_FILES) | ||
| @echo ">> building script.js" | ||
| @$(DOCKER_CMD) elm make src/Main.elm --yes --output $(TEMPFILE_JS) | ||
| @$(DOCKER_CMD) uglifyjs $(TEMPFILE_JS) --compress unused --mangle --output $(@) | ||
| @$(DOCKER_CMD) rm -rf elm-stuff |
There was a problem hiding this comment.
This forces us to re-download all the dependencies on each build, right? Is this necessary?
There was a problem hiding this comment.
I know, I had some weird errors with the messed up elm-stuff, perhaps because elm-test does some weird things with test-dependencies, it's still pretty fast though.
There was a problem hiding this comment.
Ok, sounds good to me. We can still change it, in case it is worth optimizing.
| else | ||
| Nothing | ||
|
|
||
|
|
||
| dateFormat : Time.Time -> String | ||
| dateFormat = | ||
| Date.fromTime >> (Date.Extra.Format.formatUtc config Date.Extra.Format.isoDateFormat) |
There was a problem hiding this comment.
What is the reasoning for partially implementing our own date time library?
There was a problem hiding this comment.
It is pretty small, the only bits that I added are for formatting. I don't want to pull an extra library for this because of our small requirements. We used two third party packages plus the core Date package, now it is only Iso8601 for parsing/stringifying and the core Time packages.
| import Utils.String | ||
|
|
||
|
|
||
| tab : tab -> tab -> (tab -> msg) -> List (Html msg) -> Html msg | ||
| tab tab currentTab msg content = | ||
| tab tab_ currentTab msg content = |
There was a problem hiding this comment.
Just for my understanding, what is the convention in <variable-name>_?
There was a problem hiding this comment.
Variable shadowing is disallowed in 0.19. I've seen folks doing this instead of primes tab' that are not possible anymore.
There was a problem hiding this comment.
I had that question too. If that's how elm is handling it (I think shadow' was carried over from haskell?), that's fine by me.
There was a problem hiding this comment.
In some cases it is just hard to come with names. Probably the function has to become a verb, like viewTab. We can change this as we work on it in the future.
|
An alpha sounds like a good idea, this will get (hopefully) more eyes on it than just me clicking around |
There was a problem hiding this comment.
@stuartnelson3 @w0rm sounds good to me. Feel free to merge. I will follow up with the alpha release preparation then.
|
@mxinden oh just pressed merge and missed your comment |
That's perfect, thanks! |
|
@mxinden GitHub combined all the commits, seems like it's just as you requested |
Is anybody willing to do a full QA, @stuartnelson3 maybe? 🎉
Closes #1524