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

[CHANGES] Rewritten display of alerts and silences to use vuejs #124

Merged
merged 4 commits into from
Feb 13, 2019

Conversation

kfdm
Copy link
Collaborator

@kfdm kfdm commented Feb 6, 2019

  • Initial work to move from jquery to vuejs for more straightforward code
  • Fetching alerts and silences uses javascript fetch api
  • More client side rendering results in smaller requests
  • Uses datetime-local for datepicker. Currently this only works at chrome
    but will consider looking at something for firefox and chrome support
    in the near future

There is a slight regression in the since that Firefox and Safari do not properly support datetime-local for input fields but since this PR has already gotten rather large, they will be handled in a future pull request

@kfdm kfdm added the wip label Feb 6, 2019
@codecov-io
Copy link

codecov-io commented Feb 6, 2019

Codecov Report

Merging #124 into master will increase coverage by 0.41%.
The diff coverage is 53.73%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #124      +/-   ##
=========================================
+ Coverage   44.69%   45.1%   +0.41%     
=========================================
  Files          38      38              
  Lines        2394    2361      -33     
=========================================
- Hits         1070    1065       -5     
+ Misses       1324    1296      -28
Impacted Files Coverage Δ
promgen/views.py 48.99% <ø> (+1.81%) ⬆️
promgen/prometheus.py 53.71% <100%> (+0.58%) ⬆️
promgen/proxy.py 27.81% <42.3%> (+6.03%) ⬆️
promgen/forms.py 92.63% <90.9%> (ø) ⬆️

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 edede37...131962c. Read the comment docs.

kfdm added 2 commits February 13, 2019 10:43
* Initial work to move from jquery to vuejs for more straightforward code
* Fetching alerts and silences uses javascript fetch api
* More client side rendering results in smaller requests
* Uses datetime-local for datepicker. Currently this only works at chrome
  but will consider looking at something for firefox and chrome support
  in the near future
@kfdm kfdm removed the wip label Feb 13, 2019
@kfdm kfdm requested review from Asuforce and m-tkg February 13, 2019 02:04
<dd>
<ul class="list-inline">
<li v-for="(value, label, index) in alert.labels">
<a @click.prevent="silenceAppendLabel" class="label label-warning" v-bind:data-label="label" v-bind:data-value="value">{{label}}:{{value}}</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Since v-on is omitted, it is better to omit v-bind.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

v-on has a shortcut as @ so @click.prevent is the same as v-on:click.prevent. The v-bind:data-label are so I can read those values from the click event

Copy link
Contributor

Choose a reason for hiding this comment

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

I know that v-bind also has a shortcut.

https://vuejs.org/v2/guide/syntax.html#v-bind-Shorthand

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, yes I should probably be more consistent with using short forms or not. Do you have a preference for short vs long forms ?

Copy link
Contributor

@ktarow ktarow Feb 13, 2019

Choose a reason for hiding this comment

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

I like short form better.
However, if the long form is used in the project, i will use that.
Short form was used in this PR, so I wrote this comment.

<td>
<ul class="list-inline">
<li v-for="match in silence.matchers">
<a @click.prevent="silenceAppendLabel" class="label label-warning" v-bind:data-label="match.name" v-bind:data-value="match.value">{{match.name}}:{{match.value}}</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

same as promgen/templates/promgen/alert_row.html#L11

Copy link

@m-tkg m-tkg left a comment

Choose a reason for hiding this comment

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

seems good.

Copy link
Member

@Asuforce Asuforce left a comment

Choose a reason for hiding this comment

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

Check this please

@@ -0,0 +1,8 @@
{% for message in messages %}
<div class="alert {% if message.tags %}alert-{{ message.tags }}{% endif %}" role="alert">
Copy link
Member

Choose a reason for hiding this comment

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

ref: https://vuejs.org/v2/guide/class-and-style.html#Object-Syntax
This is a little difficult to understand, so how about this ?

Suggested change
<div class="alert {% if message.tags %}alert-{{ message.tags }}{% endif %}" role="alert">
<div class="alert" :class="{ alertMessageTags }" role="alert">

and you can write such a property.

computed: {
  alertMessageTags: function () {
    return {
      `alert-${message.tags}`: this.message.tags
    }
  }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, that part is actually still django template code it's unfortunate that the vue js and django templates look the same (so in some places I had to have django ignore vuejs blocks)

Copy link
Member

Choose a reason for hiding this comment

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

ok I understand.

this.fetchSilences();
}
})

Copy link
Member

Choose a reason for hiding this comment

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

del line

promgen/proxy.py Outdated
return HttpResponse(
response.text, status=response.status_code, content_type="application/json"
)

Copy link
Member

Choose a reason for hiding this comment

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

del line

</button>
{{ message }}
</div>
{% endfor %}
Copy link
Member

Choose a reason for hiding this comment

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

add line

<button class="btn btn-primary">{% trans "Subscribe to Notifications" %}</button>
</form>
</div>
</div>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

add line

Copy link
Member

@Asuforce Asuforce left a comment

Choose a reason for hiding this comment

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

LGTM

@kfdm kfdm changed the title Vue silence [CHANGES] Rewritten display of alerts and silences to use vuejs Feb 13, 2019
@kfdm kfdm merged commit 7d7725a into line:master Feb 13, 2019
@kfdm kfdm deleted the vue-silence branch February 15, 2019 07:07
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.

5 participants