-
Notifications
You must be signed in to change notification settings - Fork 36
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
Menu item to view a community in a specific instance other than the originating instance #254
Menu item to view a community in a specific instance other than the originating instance #254
Conversation
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 looks really really good. Nice work on a big challenge.
Most of the changes are around updating jwt to user data which I love cause it gives us a lot of room to scale.
Sorry to be this guy but given that this is such a critical piece of our app, we need to add some unit tests that test the useLoggedInAction functionality and also your new subscribe capability
… JWT The app mostly just passes the JWT token around, which is ok for auth but is not ok for identification. This modifies a lot of files but all it really does is refactors the controllers to use UserData instead of just the JWT token directly. This will allow us to identify which user the request is being made for, rather than just blindly passing auth data. This will be more useful in later commits.
fce3850
to
387166d
Compare
@zachatrocity There's actually no additional functionality added to the subscribe button other than opening the menu for non-logged in instances. I also don't think there should be because it would abstract away the fact that it's not on their instance. Noted on the useLoggedInAction hook |
I see, really usedLoggedInAction should have had a test before. I'll approve, having a test would be nice but we don't have much scaffolding out there. |
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.
LGTM, just need to fix the lint
This adds a new menu item which appears under the "..." menu of a community called "View on another instance".
This pops up a list of accounts for you to choose from. The "Subscribe" button, when viewing anonymously, also pops this up:
This then executes a ResolveObject call to retrieve the community:
Before showing you the community, in the other instance. Denoted by the "(via instance)" text in the name of the community.
You can then subscribe directly from here, or if some posts are loaded you can even comment or vote directly.
Dependent on #261