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

203 notification events view #223

Merged
merged 13 commits into from
Nov 9, 2023
Merged

Conversation

jmbrunskill
Copy link
Collaborator

@jmbrunskill jmbrunskill commented Nov 3, 2023

Closes #203

πŸ‘©πŸ»β€πŸ’» What does this PR do?

Adds a Event Log View to allow administrators to see what messages were sent and any errors...

Filter By Status:

Kapture 2023-11-06 at 15 25 41

Filter By Time Range:
Kapture 2023-11-06 at 15 26 31

Search:
Kapture 2023-11-06 at 15 28 14

Navigate to detailed view, and on to notification configuration:

Kapture 2023-11-06 at 15 30 22

Detailed View (SENT):

image

Detailed View (FAILED):

image

πŸ§ͺ How has/should this change been tested?

Did some local testing.

  • Test filtering & sorting

πŸ’Œ Any notes for the reviewer?

  • I haven't done an arbitrary date range option, and there would be a possibility for more filters. I this is a good MVP for now.
  • I haven't bothered with translations for now as it's a very admin focussed screen.

@jmbrunskill jmbrunskill linked an issue Nov 3, 2023 that may be closed by this pull request
6 tasks
Copy link

github-actions bot commented Nov 3, 2023

Bundle size difference

Comparing this PR to main

Old size New size Diff
2.11 MB 2.12 MB 7.31 KB (0.34%)

@jmbrunskill jmbrunskill marked this pull request as ready for review November 6, 2023 02:56
@mark-prins mark-prins self-assigned this Nov 7, 2023
Copy link
Contributor

@mark-prins mark-prins left a comment

Choose a reason for hiding this comment

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

πŸŽ‰ Lots of great stuff there! So good to have a list of events easily viewable.
Couple of general comments: I would use a modal for the detail view, rather than a new page - there's nothing in the nav when you are on the detail page. The Events menu isn't highlight and it's like you are in some strange limbo..

Is the edit list only for events? so the edit button always applies?

The filters - I find it strange having them spaced across the width of the page. I'd jam them all together, or using the omSupply FilterBar component.

EventStatus,
} from '@notify-frontend/common';

export const FilterBar = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

this would benefit from taking the new FilterBar from omSupply and implementing the filters from there

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mark-prins Do you have a link to the New FilterBar? There doesn't seem to be a common component called that?

Copy link
Contributor

Choose a reason for hiding this comment

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

the work has been done on the release candidate branch - sorry, was going to chat to you about this one!

There's various PRs demonstrating the usage - you could try the location filter one as start maybe?

@jmbrunskill
Copy link
Collaborator Author

Is the edit list only for events? so the edit button always applies?

You mean this button right?

image

It's possible (from the database perspective) for an event to be created without linking to a config, but i think in practice is shouldn't really happen.

@jmbrunskill
Copy link
Collaborator Author

Couple of general comments: I would use a modal for the detail view, rather than a new page - there's nothing in the nav when you are on the detail page. The Events menu isn't highlight and it's like you are in some strange limbo..

A lot of the app navigates to a new page for a detail view. Maybe we just need to fix the highlighting?
Obviously a lot can and should be done to improve this, but given time constraints I just want to get it out there for now, even if it's a bit weird :)

@jmbrunskill jmbrunskill merged commit 9a67ce1 into main Nov 9, 2023
@jmbrunskill jmbrunskill deleted the 203-notification-events-view branch November 9, 2023 02:35
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.

Notification Events View
2 participants