Add application layout components #1031
Conversation
|
Demo starting at https://react-components-1031.demos.haus |
There was a problem hiding this comment.
Read through all the ApplicationLayout components and left some comments. Those seem very useful and well composable, thanks for adding them!
I made just a rough review of the Panel and SideNavigation components. Maybe factor them into a dedicated PR to make it easier to review? I think those two are a bit more controversial and might need more detailed alignment and discussions.
The demo build is failing due to:
13.43 src/components/ApplicationLayout/AppAside/AppAside.tsx(37,8): error TS2322: Type '{ children: ReactNode; controls: Element; contentClassName?: string; className?: string; dark?: boolean; wrapContent?: boolean; forwardRef?: Ref<HTMLDivElement>; ... 6 more ...; stickyHeader?: never; } | { ...; } | { ...; }' is not assignable to type 'IntrinsicAttributes & Props<LogoDefaultElement>'.
13.43 Type '{ children: ReactNode; controls: Element; contentClassName?: string; className?: string; dark?: boolean; wrapContent?: boolean; forwardRef?: Ref<HTMLDivElement>; ... 6 more ...; stickyHeader?: never; }' is not assignable to type 'IntrinsicAttributes & Props<LogoDefaultElement>'.
13.43 Type '{ children: ReactNode; controls: Element; contentClassName?: string; className?: string; dark?: boolean; wrapContent?: boolean; forwardRef?: Ref<HTMLDivElement>; ... 6 more ...; stickyHeader?: never; }' is not assignable to type 'LogoProps<LogoDefaultElement>'.
13.43 Property 'logo' is optional in type '{ children: ReactNode; controls: Element; contentClassName?: string; className?: string; dark?: boolean; wrapContent?: boolean; forwardRef?: Ref<HTMLDivElement>; ... 6 more ...; stickyHeader?: never; }' but required in type 'LogoProps<LogoDefaultElement>'.
src/components/ApplicationLayout/AppNavigationBar/AppNavigationBar.tsx
Outdated
Show resolved
Hide resolved
| } | ||
| >; | ||
|
|
||
| const ApplicationLayout = < |
There was a problem hiding this comment.
I am not sure a combination of the other components is generic enough to have it in react-components. I'd assume our different applications have too different requirements to allow a reuse of this combination. Wdyt?
There was a problem hiding this comment.
This component has been working for the apps that I've tested in so far (ReBAC Admin, Juju Dashboard and Admin UI), so it's possible we can use this for common cases and then some apps might need to have their own implementation.
I plan to keep testing this with other apps to see where it doesn't meet our needs.
There was a problem hiding this comment.
It looks like MAAS-UI and and LXD-UI have some custom menu features that mean that the <ApplicationLayout> component doesn't quite work, or at least not without making it more complicated. In both these cases they can be composed using the child components.
There was a problem hiding this comment.
Turns out LXD just needed to pass a single class and it could use the ApplicationLayout component.
huwshimi
force-pushed
the
add-application-layout
branch
from
60a2e4d
to
9a2d671
Compare
That's a good idea, I might split this up into a number of PRs once I have it all working together. |
|
This PR is being prevented from merging because it needs to be reviewed on Percy. Go to Percy, find the build relevant to this PR and check if it looks as expected. Once it's approved, add the label |
|
Here are some tests I've conducted to see how these components would work in our apps: canonical/lxd-ui#768 |
Creating this draft in case anyone is planning to do something similar. I haven't had time to finish this, but these components are used in Juju Dashboard and ReBAC Admin.
TODO