-
Notifications
You must be signed in to change notification settings - Fork 1
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
refactor the dynamic sidebar links (menuLinks) to be extendable to control all dashboard links #130
refactor the dynamic sidebar links (menuLinks) to be extendable to control all dashboard links #130
Conversation
* Changes the library from SidebarItems to DashboardLinks (to make it easier to extend the lib to manage other link types) * Changes the library to use Dashboard nomenclature * Refactors charm and tests to use new library * Renames any existing sidebar_items to menu_links
* Takes the existing menuLink (previously, sidebar) tooling in the charm and extracts it into a generic dashboard_links.py file that is link-type agnostic. * reimplements the menuLinks using the new link-type agnostic tooling * updates tests to new structure
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.
refactoring looks good, left one small comment
def aggregate_links( | ||
links_from_relation: List[DashboardLink], additional_link_config: str, link_order_config: str | ||
): |
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.
docstring missing here, same for the method below
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.
added!
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 good job
f211207
into
kf-3664-dynamic-sidebar-feature-branch
…ntrol all dashboard links (#130) Takes the existing sidebar implementation (for dashboard `menuLinks`) and refactors it to be easily extended to all kubeflow dashboard link types (menu, external, quick, and documentation). * Changes the library from SidebarItems to DashboardLinks (to make it easier to extend the lib to manage other link types) * Changes the library to use Dashboard nomenclature * Refactors charm and tests to use updated library * Renames any existing sidebar_items to menu_links
This PR refactors the existing dynamic dashboard implementation to make it more easily reused with other dashboard link types.
changes include:
The general function of the charm shouldn't change, just the implementation.
Notes to reviewers: