-
Notifications
You must be signed in to change notification settings - Fork 38
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
Platform-independent frontend slots #940
base: master
Are you sure you want to change the base?
Conversation
god this sucks so much
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
// returns React content to be rendered in the slot | ||
export type PlatformSlotContent<Location extends keyof PlatformSlotDetails> = ComponentType<{ | ||
details: PlatformSlotDetails[Location]; | ||
location: Location; |
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.
location
is a global name why did i think it was a good idea to name a parameter that. call it slotName
or something
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Wanted to do some work on this, but because slot contents are rendered in the shadow DOM now, legacy styles don't work with this PR at all. Putting a I've gotten this up-to-date with current master again but I think I'm going to continue not working on this until all the slottable UI stuff is reworked to use component styles. We can maybe get away with not doing literally everything first, since windows and stuff we can just insert into the bottom of |
okay i lied i got a little bit of shreddit working, enough to show that this will actually work there in the future. the focus here should still not be on that yet though - we still need parity on all the other platforms before i can put a ton of effort into the shreddit observer. i just wanted to throw together a proof of concept that assures me all this work isn't for nothing branch compare with a partially working shreddit observer: compare |
new Reddit is dead (#1027) so rip the newreddit observer. brought up to date with |
Basically
TBListener
but beefier - a proper solution for defining platform-independent locations where UI elements can be placed by modules.frontend/index.tsx
defines a set of locations where modules might want to place buttons, regardless of platform. It also defines a standardized set of context information available to each location. It exposes a function modules can use to have React components rendered at those locations, and it exposes types used by platform-specific observers to set where each location is rendered within that specific frontend.frontend/newreddit.tsx
defines the observer for new Reddit. It's the simplest to implement by far, because the elements are mostly already provided by the new Reddit jsAPI; we just insert new renderers into the slots we receive. Observers for other platforms are stubs for now but will be implemented before this PR is ready to merge (with the exception of shreddit, which will happen in its own PR).The end goal here is to replace
TBListener
with this new system. TBListener handles new Reddit via jsAPI only, and old Reddit is basically handled by spoofing TBListener events so that we can pretend old Reddit has jsAPI. Extending this system to Shreddit presents challenges, because we need an entirely new system to scrape the required information from the shreddit frontend before emitting it as events, and the actual data models provided by jsAPI are not super well defined.Fixes #865. Shreddit observer will be handled separately, after this PR, as a fix for #864.
Other observers should be implemented before doing more of thisthat's done so I can do this now