-
Notifications
You must be signed in to change notification settings - Fork 14
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 rudimentary notificationprofile support #1095
Add rudimentary notificationprofile support #1095
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1095 +/- ##
==========================================
- Coverage 80.04% 79.82% -0.22%
==========================================
Files 143 144 +1
Lines 5206 5250 +44
==========================================
+ Hits 4167 4191 +24
- Misses 1039 1059 +20 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delivers what it promises. Agree with a list of todos in #1095 (comment), we need separate issues for them. See some suggestions, typo fixes below.
Also tested it manually and it works mostly as expected, except wrong urls in some buttons (see typo fixes below) and a bug: it is possible to create profile with no destinations.
src/argus/htmx/templates/htmx/notificationprofile/_notificationprofile_detail.html
Outdated
Show resolved
Hide resolved
src/argus/htmx/templates/htmx/notificationprofile/_notificationprofile_detail.html
Outdated
Show resolved
Hide resolved
src/argus/htmx/templates/htmx/notificationprofile/_notificationprofile_detail.html
Show resolved
Hide resolved
…nprofile_detail.html Co-authored-by: podliashanyk <[email protected]>
…nprofile_detail.html Co-authored-by: podliashanyk <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See a little cleanup below. Also styles.css seem to need an update
src/argus/htmx/templates/htmx/notificationprofile/notificationprofile_form.html
Outdated
Show resolved
Hide resolved
…profile_form.html Co-authored-by: podliashanyk <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done! 🙌
Can be merged after updated styles are cleaned up a little. Seems like there is a bunch of breadcrumbs
styles added yet we don't use them in this PR yet.
3846182
to
6ebbba5
Compare
Quality Gate passedIssues Measures |
There are breadcrumbs-classes in use, in the admin, see Unless there is a way to make tailwind ignore some templates I think we are stuck with this. |
Oh right.. Seems like an update of |
Same technique used as for timeslots, very fugly, but will take less work to look good!