-
Notifications
You must be signed in to change notification settings - Fork 42
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
Dashboard: microapp v2 #999
Conversation
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
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.
@@ -34,6 +34,9 @@ | |||
"pnpm": { | |||
"overrides": { | |||
"typescript-json-schema>@types/node": "*" | |||
}, | |||
"patchedDependencies": { |
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.
Thanks for adding this in, should we add some comments about this patch as well?
I'm not very familiar with patches in this manner, the .patch
file was written by you manually, is that right? Or was it recommended somewhere?
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.
It was a known problem with mui and three-fiber, the only solution I found is to do a hacky patch.
To addon, I've found that pages (tabs) are no longer contained in the viewport, and users are required to scroll, to view the lift table under |
Yeah, we no longer force the height of the workspace. But it is still possible to put all the micro apps without scrolling if you change the layout. |
Signed-off-by: Teo Koon Peng <[email protected]>
Thanks for helping to test, should be fixed now |
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.
Just these 2 questions/comments, but otherwise LGTM
</ListItemIcon> | ||
</MenuItem> | ||
<Divider /> | ||
{/* <MenuItem |
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.
Was the admin actions menu item left out for a reason?
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.
It's just testing left overs, uncommented it now.
{ | ||
name: 'Custom', | ||
route: 'custom', | ||
element: ( | ||
<LocallyPersistentWorkspace | ||
defaultWindows={[]} | ||
allowDesignMode | ||
appRegistry={appRegistry} | ||
storageKey="custom-workspace" | ||
/> | ||
), | ||
}, |
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 we'll need to find a way to toggle the custom tab based on APP_CONFIG_ENABLE_CUSTOM_TABS
, as well as the admin tab with APP_CONFIG_ENABLE_ADMIN_TAB
Or was the whole idea to remove the config variables, and let users set up the dashboard however they want with this new way? If so, then the admin tab should also be added in
or is the admin tab left out because we haven't worked out authz and actions mapping properly?
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.
Yeah the plan is to have no build time config variables, after this transitions to a framework library, we can't have build time variables unless we integrate the build system into the framework as well.
Whether to have custom tabs can be easily configured by downstream with allowDesignMode
, they can also configure the number of custom tabs and if "builtin" tabs can be customized.
The admin tab is more tricky, until the backend supports proper authz, we should never show the admin tab. But when it does support authz, we should always show the admin tab if the user is an admin. The problem is that in terms of DX, it may be disruptive to force include a tab that the user did not configure. Most likely it will not be a "tab" and instead be accessed from the user icon similar to the admin panel.
Signed-off-by: Teo Koon Peng <[email protected]>
LGTM so far! With these changes I think we'll need to start documenting how the dashboard app can be configured overall, could you also help add that in? |
There are some docs added in #1007. https://github.com/open-rmf/rmf-web/tree/koonpeng/merge-react-components/packages/rmf-dashboard-framework/docs, haven't really fully tested them in practice, would appreciate if you can help beta test it! |
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.
Thanks for pointing to the WIP docs, I've done a first pass on it so far and it looks good. We'll defer the update of documentation to #1007 then.
LGTM! Thanks!
These are changes to the "microapp" architecture to make it more modular. The goal is to eventually combine
react-components
and the current dashboard and make it into a framework from which new dashboards can be easily created.Configurable Microapps
Previously microapp is a "one and done" thing where there is no way to customize them. Now microapps can have customization options via a factory method.
foo.tsx
foo-app.ts
The fourth argument to
createMicroApp
is a callback that returns the props that will be passed to the component when rendering. The map app is one example where the pattern is used.MicroApp with Settings
On top of configurable micro apps, they can now also store runtime settings, in the
props
callback increateMicroApp
, it receives a settings object, which has amicroAppSettings: { [k: string]: unknown }
field. Micro apps can store their settings there under their appId.Lazy loaded Microapps
All microapps are now lazy loaded, notice above that when creating the foo app, the
Foo
component is loaded via a dynamic import. This allows vite to code split the micro apps and load only the microapps that are currently being used in a workspace.Currently this results in reduction of the largest bundle from ~3mb to ~1.5mb.
Deferred Contexts
Normally contexts must always have a default value, in some cases, we may have a context that must always be available but it is not possible to set a default value. Previously the solution is to use
null
as the default value but through the blessing and curse of typescript, it means that we must have a non null assertion everywhere we use it.RmfIngress
is one such example where it creates a lot of boilerplate.Deferred contexts is a convenient wrapper that does the assertions. It will throw an error if no value is provided to the context yet.
Easy dashboard customization
With the new microapps architecture, a new dashboard can be easily created with minimal code. Below is an example to create a minimal dashboard with a map, doors, lifts, robots and mutex groups app.
Theming
Previously, themes are fixed, the only way to change the theme is to change the dashboard itself. With the move to a dashboard framework architecture, it is now possible to create dashboards with different themes. The
examples/custom-theme
shows an example of a dashboard with custom theme.