-
Notifications
You must be signed in to change notification settings - Fork 12
chore: un-generic panel display components #95
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,14 @@ | ||
| import { type ReactNode } from "react"; | ||
| import { createStyles, makeStyles, Theme } from "@material-ui/core/styles"; | ||
| import "react-reflex/styles.css"; | ||
| import { DataStore } from "../../../services/datastore"; | ||
| import { Services } from "../../../services/services"; | ||
| import { ReflexedPanelLocation } from "../types"; | ||
|
|
||
| /** | ||
| * Panel defines a single panel found in the panel display component. | ||
| */ | ||
| export interface Panel<L extends string> { | ||
| export interface Panel { | ||
| /** | ||
| * id is the unique ID for the panel. Must be stable across loads. | ||
| */ | ||
|
|
@@ -15,29 +17,29 @@ export interface Panel<L extends string> { | |
| /** | ||
| * summary is the React tag to render for displaying the summary of the panel. | ||
| */ | ||
| summary: (props: PanelSummaryProps<L>) => JSX.Element; | ||
| Summary: (props: PanelSummaryProps) => ReactNode; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Capsing here means we don't have to do a rename elsewhere. |
||
|
|
||
| /** | ||
| * content is the React tag to render for displaying the contents of the panel. | ||
| */ | ||
| content: (props: PanelProps<L>) => JSX.Element; | ||
| Content: (props: PanelProps) => ReactNode; | ||
| } | ||
|
|
||
| /** | ||
| * PanelProps are the props passed to all panels content tags. | ||
| */ | ||
| export interface PanelProps<L extends string> { | ||
| export interface PanelProps { | ||
| datastore: DataStore; | ||
| services: Services; | ||
| location: L; | ||
| location: ReflexedPanelLocation; | ||
| } | ||
|
|
||
| /** | ||
| * PanelSummaryProps are the props passed to all panel summary tags. | ||
| */ | ||
| export interface PanelSummaryProps<L extends string> { | ||
| export interface PanelSummaryProps { | ||
| services: Services; | ||
| location: L; | ||
| location: ReflexedPanelLocation; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,15 +13,16 @@ import { DataStore } from "../../../services/datastore"; | |
| import { Services } from "../../../services/services"; | ||
| import { Panel, useSummaryStyles } from "./common"; | ||
| import { LocationData, PanelsCoordinator } from "./coordinator"; | ||
| import { ReflexedPanelLocation } from "../types"; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the non-generic type. I'll call out its definition later. |
||
|
|
||
| export const SUMMARY_BAR_HEIGHT = 48; // Pixels | ||
|
|
||
| /** | ||
| * PanelSummaryBar is the summary bar displayed when a panel display is closed. | ||
| */ | ||
| export function PanelSummaryBar<L extends string>(props: { | ||
| location: L; | ||
| coordinator: PanelsCoordinator<L>; | ||
| export function PanelSummaryBar(props: { | ||
| location: ReflexedPanelLocation; | ||
| coordinator: PanelsCoordinator; | ||
| services: Services; | ||
| disabled?: boolean | undefined; | ||
| overrideSummaryDisplay?: ReactNode; | ||
|
|
@@ -45,10 +46,10 @@ export function PanelSummaryBar<L extends string>(props: { | |
| > | ||
| {props.overrideSummaryDisplay !== undefined && | ||
| props.overrideSummaryDisplay} | ||
| {panels.map((panel: Panel<L>) => { | ||
| {panels.map((panel: Panel) => { | ||
| // NOTE: Using this as a tag here is important for React's state system. Otherwise, | ||
| // it'll run hooks outside of the normal flow, which breaks things. | ||
| const Summary = panel.summary; | ||
| const Summary = panel.Summary; | ||
| return ( | ||
| <Button | ||
| key={panel.id} | ||
|
|
@@ -114,23 +115,20 @@ const TOOLBAR_HEIGHT = 48; // Pixels | |
| /** | ||
| * PanelDisplay displays the panels in a specific location. | ||
| */ | ||
| export function PanelDisplay<L extends string>( | ||
| props: { | ||
| location: L; | ||
| coordinator: PanelsCoordinator<L>; | ||
| datastore: DataStore; | ||
| services: Services; | ||
| } & { | ||
| dimensions?: { width: number; height: number }; | ||
| }, | ||
|
Comment on lines
-118
to
-125
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also flattened all of these types for the sake of simplicity. |
||
| ) { | ||
| export function PanelDisplay(props: { | ||
| location: ReflexedPanelLocation; | ||
| coordinator: PanelsCoordinator; | ||
| datastore: DataStore; | ||
| services: Services; | ||
| dimensions?: { width: number; height: number }; | ||
| }) { | ||
| const classes = useStyles(); | ||
| const coordinator = props.coordinator; | ||
|
|
||
| const currentTabName = coordinator.getActivePanel(props.location)?.id || ""; | ||
|
|
||
| const handleChangeTab = useCallback( | ||
| (_event: React.ChangeEvent<object>, selectedPanelId: string) => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unused so why bother |
||
| (_event: object, selectedPanelId: string) => { | ||
| coordinator.setActivePanel(selectedPanelId, props.location); | ||
| }, | ||
| [coordinator, props.location], | ||
|
|
@@ -159,23 +157,16 @@ export function PanelDisplay<L extends string>( | |
| aria-label="Tabs" | ||
| variant="fullWidth" | ||
| > | ||
| {panels.map((panel: Panel<L>) => { | ||
| {panels.map(({ id, Summary }: Panel) => { | ||
| // NOTE: Using this as a tag here is important for React's state system. Otherwise, | ||
| // it'll run hooks outside of the normal flow, which breaks things. | ||
| const Summary = panel.summary; | ||
| return ( | ||
| <Tab | ||
| key={`tab-${panel.id}`} | ||
| value={panel.id} | ||
| label={<Summary {...props} />} | ||
| /> | ||
| ); | ||
| return <Tab key={id} value={id} label={<Summary {...props} />} />; | ||
| })} | ||
| </Tabs> | ||
|
|
||
| <span> | ||
| {currentTabName && | ||
| coordinator.listLocations().map((locData: LocationData<L>) => { | ||
| coordinator.listLocations().map((locData: LocationData) => { | ||
| if (locData.location === props.location) { | ||
| return <div key={locData.location} />; | ||
| } | ||
|
|
@@ -215,27 +206,26 @@ export function PanelDisplay<L extends string>( | |
| </Toolbar> | ||
| </AppBar> | ||
|
|
||
| {panels.map((panel: Panel<L>) => { | ||
| {panels.map(({ id, Content }: Panel) => { | ||
| // NOTE: Using this as a tag here is important for React's state system. Otherwise, | ||
| // it'll run hooks outside of the normal flow, which breaks things. | ||
| const Content = panel.content; | ||
| const height = | ||
| (props.dimensions?.height ?? 0 >= 48) | ||
| ? (props.dimensions?.height ?? 0) - 48 | ||
| : "auto"; | ||
|
|
||
| return ( | ||
| <TabPanel | ||
| key={`panel-${panel.id}`} | ||
| index={panel.id} | ||
| key={id} | ||
| index={id} | ||
| value={currentTabName} | ||
| style={{ | ||
| overflow: "auto", | ||
| height: height || "auto", | ||
| position: "relative", | ||
| }} | ||
| > | ||
| {currentTabName === panel.id && <Content {...contentProps} />} | ||
| {currentTabName === id && <Content {...contentProps} />} | ||
| </TabPanel> | ||
| ); | ||
| })} | ||
|
|
||
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.
Most of these changes are un-genericking all of these
Ls. We only have a single instance of theReflexedPaneltop-level component, so there's no reason to propagate all of these generics through.