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.

Sign up for GitHub

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

Jump to bottom

Event and Incident History improvements #166

Merged
merged 12 commits into from
May 8, 2024
Merged

Event and Incident History improvements #166

merged 12 commits into from
May 8, 2024

Conversation

ncosta-ic
Copy link
Member

@ncosta-ic ncosta-ic commented Mar 25, 2024 •
edited
Loading

targets #157

@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Mar 25, 2024
@ncosta-ic ncosta-ic requested a review from nilmerg March 25, 2024 09:56
@ncosta-ic ncosta-ic linked an issue Mar 25, 2024 that may be closed by this pull request
Events and Incident History Improvements #157
Closed
@raviks789 raviks789 mentioned this pull request Mar 26, 2024
Merged
@nilmerg nilmerg added enhancement New feature or improvement area/view Affects an entire view labels Apr 9, 2024
@nilmerg nilmerg added this to the Beta milestone Apr 9, 2024
@nilmerg nilmerg mentioned this pull request Apr 15, 2024
Open
@ncosta-ic ncosta-ic force-pushed the events-incident-history-improvements branch 2 times, most recently from 9523ced to 2e39ca2 Compare April 16, 2024 16:44
@ncosta-ic ncosta-ic removed their assignment Apr 18, 2024
@ncosta-ic ncosta-ic force-pushed the events-incident-history-improvements branch from 2e39ca2 to a0f5800 Compare April 18, 2024 10:29
@ncosta-ic ncosta-ic self-assigned this Apr 18, 2024
@nilmerg nilmerg marked this pull request as ready for review April 18, 2024 12:16
nilmerg
nilmerg requested changes Apr 18, 2024
View reviewed changes
application/controllers/IncidentController.php Outdated Show resolved Hide resolved
application/controllers/IncidentController.php Outdated Show resolved Hide resolved
application/controllers/IncidentController.php Outdated Show resolved Hide resolved
application/controllers/IncidentController.php Outdated Show resolved Hide resolved
library/Notifications/Common/Links.php Outdated Show resolved Hide resolved
library/Notifications/Widget/ItemList/IncidentHistoryListItem.php Outdated Show resolved Hide resolved
library/Notifications/Widget/ItemList/IncidentHistoryListItem.php Outdated Show resolved Hide resolved
library/Notifications/Widget/ItemList/IncidentHistoryListItem.php Outdated Show resolved Hide resolved
library/Notifications/Widget/ItemList/IncidentHistoryListItem.php Outdated Show resolved Hide resolved
public/css/common.less Show resolved Hide resolved
@Icinga Icinga deleted a comment from nilmerg Apr 19, 2024
@Icinga Icinga deleted a comment from nilmerg Apr 19, 2024
nilmerg
nilmerg requested changes Apr 25, 2024
View reviewed changes
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

Since Icinga/icinga-notifications#182 there's now the possibility to sort the incident history by time, type. Please apply this here.

image
The paperplane icon seems somewhat misplaced compared with others. It wouldn't be the first we add specific rules for to fix this. For me, this is fixed when still applying a margin-right: .2em on ::before. Which is the default anyway for icons, but is reset because it's inside a item list visual. But it's specific to the icon ball widget anyway, so I think overriding it again specifically for this is fine.

The incident opened event has an incorrect icon. It's the severity's icon.

The incident close event should, again according to @flourish86's mockup, not use circle-check but check.

library/Notifications/Model/Objects.php Outdated Show resolved Hide resolved
library/Notifications/Widget/ItemList/IncidentHistoryListItem.php Outdated Show resolved Hide resolved
library/Notifications/Widget/ItemList/IncidentHistoryListItem.php Outdated Show resolved Hide resolved
@ncosta-ic ncosta-ic force-pushed the events-incident-history-improvements branch from c26d3a6 to 4db47c4 Compare April 25, 2024 10:49
@nilmerg nilmerg mentioned this pull request Apr 25, 2024
Closed
nilmerg
nilmerg requested changes Apr 26, 2024
View reviewed changes
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

The changes in IncidentDetail made by 49c3608#diff-2a8c9a2d6041980eea6f473d3b469c29603fd7ab4b932feebe94947ffbc20be4 are now obsolete. Please undo those.

library/Notifications/Widget/ItemList/IncidentHistoryListItem.php Outdated Show resolved Hide resolved
library/Notifications/Widget/ItemList/IncidentHistoryListItem.php Outdated Show resolved Hide resolved
public/css/common.less Outdated Show resolved Hide resolved
@ncosta-ic ncosta-ic force-pushed the events-incident-history-improvements branch from dc6b10b to 3b5e50a Compare April 26, 2024 08:38
@ncosta-ic ncosta-ic mentioned this pull request Apr 26, 2024
Merged
nilmerg
nilmerg previously approved these changes Apr 29, 2024
View reviewed changes
@nilmerg
Copy link
Member

nilmerg commented Apr 29, 2024

  • Please cleanup your commits by dropping those that represent now obsolete changes
  • Please merge commits if they target the same file, especially if it's a new one you introduced and there are only styling fixes in the other commit
  • Please change your commit messages, so that the branch name isn't part of them. Git and Github impose a length limit which we shouldn't exceed regularly
  • Also, remove any prefixes that don't provide any benefit as the message also provides the same information

image

@ncosta-ic ncosta-ic force-pushed the events-incident-history-improvements branch from 349bb3a to 9a5f7d0 Compare April 29, 2024 11:59
nilmerg
nilmerg requested changes May 3, 2024
View reviewed changes
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

plus what we discussed regarding the commit history ;)

library/Notifications/Model/Incident.php Outdated Show resolved Hide resolved
library/Notifications/Model/Incident.php Outdated Show resolved Hide resolved
library/Notifications/Model/IncidentHistory.php Outdated Show resolved Hide resolved
library/Notifications/Model/Objects.php Outdated Show resolved Hide resolved
library/Notifications/Widget/ItemList/EventListItem.php Outdated Show resolved Hide resolved
@ncosta-ic ncosta-ic force-pushed the events-incident-history-improvements branch from 9a5f7d0 to 7494010 Compare May 3, 2024 16:45
@ncosta-ic ncosta-ic force-pushed the events-incident-history-improvements branch from 7494010 to c1d762a Compare May 3, 2024 16:51
nilmerg
nilmerg requested changes May 6, 2024
View reviewed changes
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

Except 0010b99#diff-7ddc625914ab7bf5145c336959fd138d5d3325193dd41a462edfb2fbfc43ff20R65, the commits are top notch now 👍

library/Notifications/Widget/Detail/IncidentDetail.php Outdated Show resolved Hide resolved
library/Notifications/Widget/ItemList/EventListItem.php Outdated Show resolved Hide resolved
library/Notifications/Widget/ItemList/IncidentHistoryListItem.php Outdated Show resolved Hide resolved
public/css/common.less Outdated Show resolved Hide resolved
@ncosta-ic ncosta-ic force-pushed the events-incident-history-improvements branch from c1d762a to b80de18 Compare May 6, 2024 09:00
@ncosta-ic ncosta-ic force-pushed the events-incident-history-improvements branch from b80de18 to 25f723d Compare May 8, 2024 08:47
@nilmerg nilmerg merged commit e044065 into main May 8, 2024
22 checks passed
@nilmerg nilmerg deleted the events-incident-history-improvements branch May 8, 2024 08:59
nilmerg added a commit that referenced this pull request May 31, 2024
@nilmerg
Hook implementation for event object display names (#165)
82bfaf7 
resolves #132 

Blocked by: #155, #166

requires: Icinga/ipl-web#217
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nilmerg nilmerg nilmerg approved these changes

Assignees

@ncosta-ic ncosta-ic

Labels
area/view Affects an entire view cla/signed CLA is signed by all contributors of a PR enhancement New feature or improvement
Projects
None yet
Milestone
Beta
Development

Successfully merging this pull request may close these issues.

Events and Incident History Improvements
2 participants
@ncosta-ic @nilmerg