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

feat(settings): user can enable/disable notifications by category #387

Merged
merged 13 commits into from
Mar 10, 2022
Merged

feat(settings): user can enable/disable notifications by category #387

merged 13 commits into from
Mar 10, 2022

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Mar 9, 2022 •
edited
Loading

Fixes #343

@andrewazores andrewazores added the feat New feature or request label Mar 9, 2022
@andrewazores andrewazores force-pushed the notification-control branch from 87f8095 to b77ada1 Compare March 9, 2022 20:32
@andrewazores andrewazores marked this pull request as ready for review March 9, 2022 20:33
@andrewazores andrewazores requested a review from hareetd March 9, 2022 20:33
@hareetd
Copy link
Contributor

hareetd commented Mar 9, 2022

I'm running a hot-reloading instance of this PR branch. When I try to create a recording in order to test the notifications, the view turns into a blank page:

image

Switched to the main branch as a sanity check and the /recordings route is working fine there. Could this be due to some sort of interaction between the altered notifications and the recordings table state?

@andrewazores
Copy link
Member Author

Could this be due to some sort of interaction between the altered notifications and the recordings table state?

I don't think so, they shouldn't interact at all.

What does your browser dev console say? Any errors?

@hareetd
Copy link
Contributor

hareetd commented Mar 9, 2022 •
edited
Loading

image

Looks like this is why but I'm confused why the metadata is causing an issue only on this branch.

@hareetd
Copy link
Contributor

hareetd commented Mar 9, 2022 •
edited
Loading

False alarm, it's because I forgot to pull in Janelle's metadata changes to my backend and frontend main branches, whereas this PR has been rebased already.

@andrewazores andrewazores force-pushed the notification-control branch from 86a338d to f5a484d Compare March 10, 2022 16:13
hareetd
hareetd reviewed Mar 10, 2022
View reviewed changes
Copy link
Contributor

@hareetd hareetd left a comment

Choose a reason for hiding this comment

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

I tested out each category and it's working well.

A minor nitpick I had is that in the Settings there are two categories named "Recording Deleted", the first for active recordings and the second for archived recordings. Do you think it's worth updating these accordingly to avoid confusion? Although that does make the naming inconsistent comparing to the other recording-related categories.

src/app/Shared/Services/Targets.service.tsx Show resolved Hide resolved
src/app/Shared/Services/Settings.service.tsx Show resolved Hide resolved
@andrewazores
Copy link
Member Author

A minor nitpick I had is that in the Settings there are two categories named "Recording Deleted", the first for active recordings and the second for archived recordings. Do you think it's worth updating these accordingly to avoid confusion? Although that does make the naming inconsistent comparing to the other recording-related categories.

Ha, nice catch. I hadn't even noticed. That would definitely be confusing for a user to try to figure out. I've updated the titles for the two notifications for archived recording actions, so it's clearer now what's happening when the notification appears and clearer when enabling/disabling those notifications.

hareetd
hareetd approved these changes Mar 10, 2022
View reviewed changes
Copy link
Contributor

@hareetd hareetd left a comment

Choose a reason for hiding this comment

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

Looks good!

@andrewazores andrewazores merged commit 12e9691 into cryostatio:main Mar 10, 2022
@andrewazores andrewazores deleted the notification-control branch March 10, 2022 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hareetd hareetd hareetd approved these changes

Assignees
No one assigned
Labels
feat New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Notification configurability
2 participants
@andrewazores @hareetd