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

expand on embedded mode #155

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

expand on embedded mode #155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 49 additions & 14 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,11 @@
"docs": "npm run dev -w docs",
"prepublishOnly": "npm run build",
"remix-vite": "npm run dev -w test-apps/remix-vite",
"remix-vite-embedded": "npm run dev -w test-apps/remix-vite-embedded",
"remix-website": "npm run dev -w test-apps/remix-website",
"runner": "npm-run-all -s build -p watch-all",
"dev": "npm run runner remix-vite",
"dev:embedded": "npm run runner remix-vite-embedded",
"dev:website": "npm run runner remix-website ",
"build": "run-s tsup:* -- --clean",
"watch-all": "npm-run-all -p tsup:index:watch tsup:client:watch tsup:server:watch -- --watch",
Expand Down
2 changes: 1 addition & 1 deletion src/client/EmbeddedDevTools.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { useLocation } from "@remix-run/react"
import clsx from "clsx"
import { useEffect, useState } from "react"
import type { RemixDevToolsProps } from "./RemixDevTools.js"
import { RDTContextProvider } from "./context/RDTContext.js"
import { RDTContextProvider, RDTEmbeddedContextProvider as RemixDevToolsEmbeddedMode } from "./context/RDTContext.js"
import { useSettingsContext } from "./context/useRDTContext.js"
import { useBorderedRoutes } from "./hooks/useBorderedRoutes.js"
import { useSetRouteBoundaries } from "./hooks/useSetRouteBoundaries.js"
Expand Down
12 changes: 11 additions & 1 deletion src/client/context/RDTContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { Dispatch } from "react"
import type React from "react"
import { createContext, useEffect, useMemo, useReducer } from "react"
import { createContext, useContext, useEffect, useMemo, useReducer } from "react"
import { useRemoveBody } from "../hooks/detached/useRemoveBody.js"
import { checkIsDetached, checkIsDetachedOwner, checkIsDetachedWindow } from "../utils/detached.js"
import { tryParseJson } from "../utils/sanitize.js"
Expand All @@ -20,6 +20,16 @@ export const RDTContext = createContext<{
dispatch: Dispatch<RemixDevToolsActions>
}>({ state: initialState, dispatch: () => null })

const RDTEmbeddedContext = createContext<boolean>(false)

export function RDTEmbeddedContextProvider({ children }: React.PropsWithChildren) {
return <RDTEmbeddedContext.Provider value={true}>{children}</RDTEmbeddedContext.Provider>
}

export function useIsEmbeddedRDT() {
return useContext(RDTEmbeddedContext)
}

RDTContext.displayName = "RDTContext"

interface ContextProps {
Expand Down
69 changes: 44 additions & 25 deletions src/client/init/root.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { useEffect, useState } from "react"
import { createPortal } from "react-dom"
import { RemixDevTools, type RemixDevToolsProps } from "../RemixDevTools.js"
import type { RdtClientConfig } from "../context/RDTContext.js"
import { RDTEmbeddedContextProvider, type RdtClientConfig, useIsEmbeddedRDT } from "../context/RDTContext.js"
import { hydrationDetector } from "./hydration.js"

let hydrating = true
Expand All @@ -19,34 +19,53 @@ function useHydrated() {

export const defineClientConfig = (config: RdtClientConfig) => config

export const withDevTools = (Component: any, config?: RemixDevToolsProps) => () => {
hydrationDetector()
const hydrated = useHydrated()
if (!hydrated) return <Component />

return (
<>
<Component />
{createPortal(<RemixDevTools {...config} />, document.body)}
</>
)
}

/**
*
* @description Injects the dev tools into the Vite App, ONLY meant to be used by the package plugin, do not use this yourself!
*/
export const withViteDevTools = (Component: any, config?: RemixDevToolsProps) => () => {
hydrationDetector()
function AppWithDevTools(props: any) {
export const withDevTools =
(Component: any, config?: RemixDevToolsProps & { panelMode?: "auto" | "embedded" }) => () => {
hydrationDetector()
const hydrated = useHydrated()
const isEmbedded = useIsEmbeddedRDT()
if (!hydrated) return <Component />

if ((config?.panelMode ?? "auto") === "embedded") {
Copy link
Contributor

Choose a reason for hiding this comment

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

why the default here? you don't really need to check if "auto" === "embedded"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mainly for clarity that the default mode is "auto". It's not technically necessary, happy to remove it if preferred.

return (
<RDTEmbeddedContextProvider>
<Component />
</RDTEmbeddedContextProvider>
)
}

return (
<>
<Component {...props} />
Copy link
Contributor

Choose a reason for hiding this comment

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

not spreading the props over the component actually breaks RR7 support, I learned it the hard way yesterday with line 27 above breaking my rr7 test setup. So props need to be accepted and spread across Component in every return case

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this withDevTools method is kind of low-key deprecated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deprecated

in favor of?

Copy link
Contributor

Choose a reason for hiding this comment

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

WithViteDevTools, this one is undocumented relic used to manually set it up if needed in remix v1, I plan to remove it fully in next major

{createPortal(<RemixDevTools {...config} />, document.body)}
<Component />
{!isEmbedded && createPortal(<RemixDevTools {...config} />, document.body)}
</>
)
}
return AppWithDevTools
}

/**
*
* @description Injects the dev tools into the Vite App, ONLY meant to be used by the package plugin, do not use this yourself!
*/
export const withViteDevTools =
(Component: any, config?: RemixDevToolsProps & { panelMode?: "auto" | "embedded" }) => () => {
hydrationDetector()
function AppWithDevTools(props: any) {
const hydrated = useHydrated()
if (!hydrated) return <Component />
if ((config?.panelMode ?? "auto") === "embedded") {
return (
<RDTEmbeddedContextProvider>
<Component />
</RDTEmbeddedContextProvider>
)
}

return (
<>
<Component {...props} />
{createPortal(<RemixDevTools {...config} />, document.body)}
</>
)
}
Comment on lines +55 to +69
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been thinking, RDTContextProvider wraps both the EmbeddedDevTools and the regular ones, why wouldn't you just pop this panelMode option into that and here just not use the createPortal? instead of having 2 contexts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Then you in the vite plugin set it to embedded, that hides this createPortal, and wherever you use you just pass in a prop that the panel is embedded (inside the EmbeddedDevTools to the context) and you get this behavior with less hassle and there is 1 context to read from

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Primarily because you really want React Context to be granular. One thing I was going to suggest to you was to separate the dispatch context out from the state context.

Having them both in the same context is inefficient, because every useContext() call subscribes the component to rerenders whenever the context value changes. That means that components that only need the dispatch will have to re-render whenever the state changes, even if they never read any state. By breaking those up into separate contexts, you get granular control of what causes a re-render. And since dispatch is stable, components that only need to dispatch won't re-render themselves after the state changes (often times in response to those components themselves invoking dispatch!).

With that in mind, having a separate context for "embedded" mode makes sense, since the value of this is essentially static, a component that needs to render differently based on embedded-mode or not won't need to re-render whenever the state managed by RDTContextProvider changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, makes sense, okay

return AppWithDevTools
}
25 changes: 17 additions & 8 deletions src/client/layout/MainPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,27 +30,36 @@ const MainPanel = ({ children, isOpen, isEmbedded = false, className }: MainPane

return (
<div
style={{
zIndex: 9998,
...(!isEmbedded && { height: detachedWindow ? window.innerHeight : height }),
}}
style={
!isEmbedded
? {
zIndex: 9998,
height: detachedWindow ? window.innerHeight : height,
}
: undefined
}
className={clsx(
"duration-600 box-border flex w-screen flex-col overflow-auto bg-main text-white opacity-0 transition-all",
"duration-600 box-border flex overflow-auto bg-main text-white opacity-0 transition-all",
isOpen ? "opacity-100 drop-shadow-2xl" : "!h-0",
isResizing && "cursor-grabbing ",
!isEmbedded ? `fixed left-0 ${panelLocation === "bottom" ? "bottom-0" : "top-0 border-b-2 border-main"}` : "",
!isEmbedded
? clsx(
"flex-col fixed left-0 w-screen",
panelLocation === "bottom" ? "bottom-0" : "top-0 border-b-2 border-main"
)
: "flex-row",
className
)}
>
{panelLocation === "bottom" && (
{!isEmbedded && panelLocation === "bottom" && (
<div
onMouseDown={enableResize}
onMouseUp={disableResize}
className={clsx("absolute z-50 h-1 w-full", isResizing ? "cursor-grabbing" : "cursor-ns-resize")}
/>
)}
{children}
{panelLocation === "top" && (
{!isEmbedded && panelLocation === "top" && (
<div
onMouseDown={enableResize}
onMouseUp={disableResize}
Expand Down
Loading
Loading