-
Notifications
You must be signed in to change notification settings - Fork 78
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
Convert Outlet
from a HOC to a standard component with a render prop
#63
Conversation
Outlet
from a HOC to a standard component with a render propOutlet
from a HOC to a standard component with a render prop
Outlet
from a HOC to a standard component with a render propOutlet
from a HOC to a standard component with a render prop
key?: RegistryLabel; | ||
mapParams?: MapParams; | ||
} | ||
export interface MatchDetails { |
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.
Is OutletDetails
a better name?
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 think MatchDetails
is more appropriate as it's the details for the current match, not the outlet as a whole. Perhaps MatchProperties
though?
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.
Don't want to overload properties
src/routing/Outlet.ts
Outdated
return Boolean(value && (typeof value === 'string' || typeof value === 'function' || typeof value === 'symbol')); | ||
export interface OutletProperties { | ||
renderer: (matchDetails: MatchDetails) => DNode; | ||
outlet: 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.
perhaps rename to id
?
src/routing/Outlet.ts
Outdated
export function isComponent<W extends WidgetBaseInterface>(value: any): value is Component<W> { | ||
return Boolean(value && (typeof value === 'string' || typeof value === 'function' || typeof value === 'symbol')); | ||
export interface OutletProperties { | ||
renderer: (matchDetails: MatchDetails) => DNode; |
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.
Can this not return an array of DNodes
?
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 think it should be able to.
decc431
to
d3877cb
Compare
528be9f
to
2e592b9
Compare
Type: feature
The following has been addressed in the PR:
prettier
as per the readme code style guidelinesDescription:
This changes the
Outlet
from a higher order component to a standard component with a render property. The render property is used to display content when a route matches providing more control over the nodes to render when an Outlet's route has been matched.Having an
Outlet
as a HOC implies that it would be shared across the application (which generally they are not), adds complexity in it's usage by having to specify another interface for the properties that theOutlet
can accept and has more boilerplate and indirection that if theOutlet
is a standard widget.Resolves #60
Alternative to #24