-
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
276c090 to
1234dec
Compare
1234dec to
07f0840
Compare
07f0840 to
cf53e4d
Compare
tstirrat15
left a comment
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.
See comments.
| /** | ||
| * Panel defines a single panel found in the panel display component. | ||
| */ | ||
| export interface Panel<L extends string> { |
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 the ReflexedPanel top-level component, so there's no reason to propagate all of these generics through.
| * summary is the React tag to render for displaying the summary of the panel. | ||
| */ | ||
| summary: (props: PanelSummaryProps<L>) => JSX.Element; | ||
| Summary: (props: PanelSummaryProps) => ReactNode; |
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.
Capsing here means we don't have to do a rename elsewhere.
| import { Services } from "../../../services/services"; | ||
| import { Panel, useSummaryStyles } from "./common"; | ||
| import { LocationData, PanelsCoordinator } from "./coordinator"; | ||
| import { ReflexedPanelLocation } from "../types"; |
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 is the non-generic type. I'll call out its definition later.
| const currentTabName = coordinator.getActivePanel(props.location)?.id || ""; | ||
|
|
||
| const handleChangeTab = useCallback( | ||
| (_event: React.ChangeEvent<object>, selectedPanelId: string) => { |
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.
Unused so why bother
| props: { | ||
| location: L; | ||
| coordinator: PanelsCoordinator<L>; | ||
| datastore: DataStore; | ||
| services: Services; | ||
| } & { | ||
| dimensions?: { width: number; height: number }; | ||
| }, |
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 also flattened all of these types for the sake of simplicity.
src/components/panels/panels.tsx
Outdated
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.
Not sure why this was here.
| export enum ReflexedPanelLocation { | ||
| HORIZONTAL = "h", | ||
| VERTICAL = "v", | ||
| } |
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 is the enum that was replaced with literals.
| datastore: DataStore; | ||
| services: Services; |
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 inlined these.
| } | ||
|
|
||
| interface DimensionsProp { | ||
| dimensions?: Dimensions | undefined; |
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.
Same with these.
| panels: props.panels, | ||
| locations: { | ||
| [ReflexedPanelLocation.HORIZONTAL]: { | ||
| horizontal: { |
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.
Taking advantage of the literals here.
cf53e4d to
9bf7e55
Compare
Description
Something I noticed moving around the repo - these components are defined in terms of generics, but they aren't used generically.
Changes
Will annotate.
Testing
Review. See that the panels still look as expected.