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

Let the browser remember the creator of a silence #1112

Merged
merged 6 commits into from
Dec 8, 2017
Merged

Let the browser remember the creator of a silence #1112

merged 6 commits into from
Dec 8, 2017

Conversation

ccmtaylor
Copy link
Contributor

@ccmtaylor ccmtaylor commented Nov 17, 2017

Currently, when a user creates silences, she needs to enter her name/email into
the creator field every time. Since the user will likely want to use the same
information every time, it makes sense to enable browser autocomplete for the
form field.

Please note that this is the first time I'm writing Elm, so let me know if I can improve things :)

Copy link
Contributor

@stuartnelson3 stuartnelson3 left a comment

Choose a reason for hiding this comment

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

Have you tested this? Checking in firefox and chromium, it doesn't seem to be working. I'm wondering if the issue has something to do with the model being updated.

@ccmtaylor
Copy link
Contributor Author

it doesn't seem to be working

welp, you're right. I tested with the dev-server, which has no CSS and mistook the comment field for an empty autocomplete suggestion from firefox 😊 . I'll read up on autocomplete specs a bit more.

@w0rm
Copy link
Member

w0rm commented Nov 17, 2017

@stuartnelson3 I only read the code and it looked fine, but apparently not working :)

@ccmtaylor I can help you with the dev server setup, so that you can get css working. I checked the autocomplete, and it seems that it is enabled by default, so it is not helpful. We can save the creator in the LocalStorage through ports and keep in the top level model.

  1. Get the creator from LocalStoage and pass it as flags to the elm program (the same way production: true is passed here and then decoded here)
  2. Put it somewhere in the model
  3. Use it to fill the input field
  4. When the form is submitted, update the creator in the model, plus send the creator through a port and update it in the LocalStorage on the JavaScript side.

@ccmtaylor
Copy link
Contributor Author

@stuartnelson3 @w0rm it semi-works for me now in Chrome and Firefox. Some experimenting suggests that browsers seem to store autocomplete values under a key that's derived from the name attribute, though I couldn't find any documentation on this behaviour on MDN.

Testing on a local build of alertmanager, it seems like the browsers suggest values that I've previously entered in name="name" fields on other sites, but they don't remember new values I enter into the Creator field in alertmanager :(. This Stackoverflow entry suggests that this is because Elm handles the form entirely in JS and doesn't actually submit the form. The workaround involves a hidden iframe; I'm not sure if this is acceptable.

@stuartnelson3
Copy link
Contributor

That's definitely an unfortunate side-effect of using elm, and I would rather not add a hidden iframe solution just to get around this.

@w0rm do you have any time to do your ports + local storage solution with @ccmtaylor ? that should be relatively few lines of code

@w0rm
Copy link
Member

w0rm commented Nov 29, 2017

@stuartnelson3 I will have time this Friday, @ccmtaylor let me know if you would like to pair on this

@ccmtaylor
Copy link
Contributor Author

thanks a bunch for helping me today, @w0rm! Really enjoyed it 🥇

Copy link
Member

@w0rm w0rm left a comment

Choose a reason for hiding this comment

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

@ccmtaylor great work! I only have a couple comments

@@ -15,6 +15,12 @@
}
</script>
<script src="script.js"></script>
<script>Elm.Main.embed(document.body, { production: true })</script>
<script>
var app = Elm.Main.embed(document.body, { production: true, defaultCreator: window.localStorage.getItem('defaultCreator') });
Copy link
Member

Choose a reason for hiding this comment

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

Could you fix the indentation here?

{ empty
| startsAt = initialField (timeToString now)
, endsAt = initialField (timeToString (now + defaultDuration))
, duration = initialField (durationFormat defaultDuration)
, createdBy = initialField (Maybe.withDefault "" defaultCreator)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I just thought that we don't really need defaultCreator to be a Maybe String and we can always keep it as an empty string if it's not set. This would make the code slightly simpler, and the decoder would be:

    |> Json.decodeValue (Json.field "defaultCreator" Json.string)
    |> Result.withDefault ""

@stuartnelson3
Copy link
Contributor

@w0rm I'll leave this PR up to you, go ahead and merge it when you're happy with how it's working!

@w0rm
Copy link
Member

w0rm commented Dec 8, 2017

@ccmtaylor great! let's wait for ci, and then merge

@w0rm w0rm merged commit af63c85 into prometheus:master Dec 8, 2017
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