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

[on hold] [pending api work] Notification settings #1186

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rafael-xmr
Copy link
Contributor

@rafael-xmr rafael-xmr commented Mar 23, 2022

Fixes

Issue Number: closes #615

@rafael-xmr rafael-xmr marked this pull request as draft March 23, 2022 13:28
@rafael-xmr rafael-xmr force-pushed the notification-settings branch from 35b3dc5 to 5ac7395 Compare March 23, 2022 16:53
@rafael-xmr rafael-xmr marked this pull request as ready for review March 23, 2022 16:54
Comment on lines 227 to 228
channel_id: '*',
channel_name: '*',
Copy link
Collaborator

@infinite-persistence infinite-persistence Mar 24, 2022

Choose a reason for hiding this comment

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

I see a channel selector, but the ID is always the same -- what's the function of the selector?

@tzarebczan

This comment was marked as resolved.

Copy link
Collaborator

@infinite-persistence infinite-persistence left a comment

Choose a reason for hiding this comment

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

Nice and clean, as always 👍
Just a couple of minor suggestions, and a 405 error mentioned above.

ui/redux/selectors/notifications.js Outdated Show resolved Hide resolved
ui/component/settingNotifications/view.jsx Outdated Show resolved Hide resolved
@rafael-xmr rafael-xmr force-pushed the notification-settings branch from 5ac7395 to e75a3f3 Compare March 24, 2022 17:55
@rafael-xmr
Copy link
Contributor Author

on https://salt.odysee.com

@infinite-persistence
Copy link
Collaborator

I think the build failed, or didn't capture the latest commit (not seeing the new Set by Channel? component)

@rafael-xmr
Copy link
Contributor Author

Sorry! now it should be

@tzarebczan tzarebczan changed the title Notification settings [pending api work] Notification settings Mar 30, 2022
@tzarebczan tzarebczan changed the title [pending api work] Notification settings [on-hold][pending api work] Notification settings Apr 8, 2022
@tzarebczan tzarebczan changed the title [on-hold][pending api work] Notification settings [on-hold] [pending api work] Notification settings Apr 8, 2022
@tzarebczan tzarebczan changed the title [on-hold] [pending api work] Notification settings [on hold] [pending api work] Notification settings Aug 4, 2022
@tzarebczan tzarebczan removed their request for review May 10, 2023 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notification settings
3 participants