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

Add the possibility of only displaying incidents in the timeline #2825

Merged
merged 1 commit into from
Mar 17, 2018

Conversation

rarguelloF
Copy link
Contributor

@rarguelloF rarguelloF commented Nov 28, 2017

I added an option to just show incidents, no matter the distance between dates, in the timeline.

For example, if you had incidents on the days 2015-2-24, 2016-2-24, and 2017-4-12, and the app_incident_days option is set to 2, you would see in the first page, all the incidents from 2017-4-12 and 2016-2-24, and in the second page (previous one), all the incidents from 2015-2-24.

This option takes for granted the Only show days containing incidents in the timeline? option (it doesn't display days without incidents).

Screenshots

  • New option in settings:

image

  • Previous example illustrated:

image

image

Please let me know if you like this feature, or if you want me to change something in the code.

Kind regards,
Rodrigo

@jbrooksuk
Copy link
Member

A lot of files have been changed when they don't need to be, things like translation files and .gitignore. Could you please undo some of these.

New translations also need to go to CrowdIn; https://translate.cachethq.io

@rarguelloF
Copy link
Contributor Author

rarguelloF commented Nov 30, 2017

Ok, thanks for your response!

One of the changes I did on the translation files was removing the space on the [0,1] and [2,Inf] text chains, since it's currently being displayed. Do you want me to create a separate PR with this (since these are not really changes on the translations themselves)?

The .gitignore change is just adding my editor stuff, do you want me to create a different PR for this?

@rarguelloF
Copy link
Contributor Author

@jbrooksuk I already removed the translation changes 👍

<div class="checkbox">
<label>
<input type="hidden" value="0" name="app_only_display_incidents">
<input type="checkbox" value="1" name="app_only_display_incidents" {{ Config::get('setting.app_only_display_incidents') ? 'checked' : null }}>
Copy link
Member

Choose a reason for hiding this comment

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

We don't use the Config facade here, instead we pass through a value, see how the rest are done 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@rarguelloF
Copy link
Contributor Author

@jbrooksuk what do you think of using the "Only show days containing incidents in the timeline?" configuration option for this behaviour?

@jbrooksuk
Copy link
Member

@rarguelloF I much prefer it :)

@rarguelloF
Copy link
Contributor Author

@jbrooksuk changes already done 👍

@jbrooksuk
Copy link
Member

Looks good! Thank you.

@jbrooksuk jbrooksuk merged commit 0fcd939 into cachethq:2.4 Mar 17, 2018
@welcome
Copy link

welcome bot commented Mar 17, 2018

Hooray! Your first Pull Request was merged, here's to many more 🚀

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.

None yet

2 participants