-
-
Notifications
You must be signed in to change notification settings - Fork 130
Add Territory Sub management tab in Subscriptions #2191
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
base: master
Are you sure you want to change the base?
Conversation
Hi brymut, I see you're working on this enhancement/feature. |
Thanks for the tip @Soxasora, its still a very rough solution. I just decided to push since I had to move on to something else at the time |
88c2832
to
ffe7be8
Compare
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.
two changes needed
export const getServerSideProps = async (ctx) => { | ||
const [userRes, subsRes] = await Promise.all([ | ||
getGetServerSideProps({ query: MY_SUBSCRIBED_USERS, authRequired: true })(ctx), | ||
getGetServerSideProps({ query: MY_SUBSCRIBED_SUBS, authRequired: true })(ctx) | ||
]) | ||
return { | ||
props: { | ||
ssrUsers: userRes.props.ssrData, | ||
ssrSubs: subsRes.props.ssrData | ||
} | ||
} | ||
} |
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.
I'm surprised this worked.
The move here is to make a compound graphql query like USER_WITH_ITEMS
or make this into different .js
files to call each query only when it is needed like we do for /top
path.
|
||
export default function MySubscribedUsers ({ ssrData }) { | ||
export default function MySubscribedUsers ({ ssrUsers, ssrSubs }) { | ||
const [view, setView] = useState('stackers') | ||
const subscribeUserContextValue = useMemo(() => ({ refetchQueries: ['MySubscribedUsers'] }), []) |
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.
This refetchQueries
pattern should be used for territory subscriptions too.
87b6571
to
395375b
Compare
395375b
to
42f3d87
Compare
Addressed |
5aa762d
to
c1994b1
Compare
c1994b1
to
abc5f72
Compare
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.
I fixed an number of things in this PR, some of them subjective, some of them bugs (like extra separators), then I encountered this dropdown bug.
The action dropdown works on the subscriptions territory page, but:
- it's still there when logged out (logged out user can't subscribe/mute)
- on the
/top/territories/forever
it thinks I'm not subscribed to any territories - same as (2) on
/<user nym>/territories
<ActionDropdown> | ||
<ToggleSubSubscriptionDropdownItem sub={sub} /> | ||
<MuteSubDropdownItem sub={sub} /> | ||
</ActionDropdown> |
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.
There are several pages with territory lists that this does not work on.
Description
Add Territory sub management in the 'subscriptions' tab in the settings page with a dropdown to switch between stackers & territories. With dropdown menu to unsubscribe from & mute territory.
Closes #1409
Screenshots
Desktop
Screen.Recording.2025-06-03.at.10.00.11.mov
Mobile
Screen.Recording.2025-06-03.at.10.03.42.mov
Additional Context
Was anything unclear during your work on this PR? Anything we should definitely take a closer look at?
Checklist
Are your changes backwards compatible? Please answer below:
Y
On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
10
For frontend changes: Tested on mobile, light and dark mode? Please answer below:
Y
Did you introduce any new environment variables? If so, call them out explicitly here:
No