-
Notifications
You must be signed in to change notification settings - Fork 139
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
feat(app) an AdminPage for admin plugins #932
Conversation
🦋 Changeset detectedLatest commit: ec2bc58 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The image is available at: |
@@ -64,6 +65,16 @@ const AppBase = () => { | |||
<Route path="/settings" element={<UserSettingsPage />} /> | |||
<Route path="/catalog-graph" element={<CatalogGraphPage />} /> | |||
<Route path="/learning-paths" element={<LearningPaths />} /> | |||
<Route path="/admin" element={<AdminPage />} /> | |||
{dynamicRoutes |
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.
as you are handling the routes here for admin and below there is a map run on all dynamicRoutes again so can that be an issue?
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 I have to revisit, as I figured I should be able to just map these pages to the admin route and then filter them from the other block (it would be before this map
) where these are mounted. But I was surprised to find it didn't work at the time, so yep this needs a closer look. I might have just confused myself with my test plugin 😄
); | ||
} | ||
if (invalidTab(selectedTab)) { | ||
// in this case the page will navigate to a tab, so avoid |
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.
which tab will it navigate to? in this case? and if am getting it right currently on rbac and plugins tab would be allowed right? in future, if any other tab needs to be added then it will require code change in this file?
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.
Correct. So this page will either navigate to the "rbac" tab or the "plugin" tab depending on what's available, and if there's no configured tab, for example let's say the configuration for all of this is removed while a user is at one of these pages then the page resorts to showing the standard error page which provides the user a link out of this area.
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.
Oh yes, and for this page additional tabs can be added by modifying this array
Ah turns out some last minute changes to fix lint errors introduced a bug, will need to fix that 😄 |
Rebased, and I have updated this to handle the case where the user might be on the admin page when there's no configuration for it, and some comments in there too. |
The image is available at: |
9b7df3a
to
15263b2
Compare
The image is available at: |
Verified it locally @gashcrumb Would you mind taking a look at the code smells ? |
Ah, of course, didn't spot that. Thanks! |
This change adds a Administration navigation tab and page intended for plugins that are for administration of an Janus/RHDH instance. This commit adds two routes /admin/rbac and /admin/plugins and related mountpoints so that dynamic plugins can contribute UI components to this page. The page can also deal with a few edge cases and will also not be visible should no plugins be configured to show up on it. A test is added to cover these edge cases. This commit also adds the dynamic-plugins-info dynamic plugin. Signed-off-by: Stan Lewis <gashcrumb@gmail.com>
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
Code smells should be sorted now... |
The image is available at: |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: invincibleJai The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thanks @gashcrumb, this looks good :) |
Description
This change adds a Administration navigation tab and page intended for plugins that are for administration of an Janus/RHDH instance. This commit adds two routes /admin/rbac and /admin/plugins and related mountpoints so that dynamic plugins can contribute UI components to this page. The page can also deal with a few edge cases and will also not be visible should no plugins be configured to show up on it. A test is provided that covers these cases. This change also adds the dynamic-plugins-info dynamic plugin which is one of two components intended to appear on this page.
Which issue(s) does this PR fix
Part of RHIDP-980. This is an approach to provide a home for dynamic plugins that fall under the "Administration" tab, for example:
PR acceptance criteria
Please make sure that the following steps are complete:
How to test changes / Special notes to the reviewer
The dynamic-plugins-info plugin is added to the showcase
To enable the Administration tab and see the installed dynamic plugins, use the following configuration in your
app-config.local.yaml
to enable the "Administration" nav and "Plugins" tab:It should be possible to browse the list of installed plugins using the pagination arrows:
and searching should work:
Removing the configuration for the tab/plugin shouldn't leave the user with a blank page: