16118 table of contents#16383
Conversation
194b8b1 to
c0de405
Compare
|
refactored to remove provied/inject and the reliance on class names. Components can now register/unregister themselves with the summary, which then searches the virtual dom to determine the relative position of registered components. Added a second composable useInSummary to reduce summary-specific logic in the accordion component. The composable may be used with other form elements as well (tested building a tree of other inputs in the imported cluster page) |
There was a problem hiding this comment.
Pull request overview
Adds an (in-progress) Table of Contents feature intended to discover Accordion components within CruResource forms and provide “jump to” navigation.
Changes:
- Introduces
TableOfContentscomponent +useFormSummary/useInSummarycomposables for locating accordions and scrolling to them. - Updates
CruResourceto optionally render the ToC via a newshowTocprop and adjusts layout styling to accommodate a sidebar. - Wires
Accordioninto the summary system and enablesshow-tocon select forms; adds global smooth scrolling.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| shell/edit/provisioning.cattle.io.cluster/rke2.vue | Enables ToC rendering on the RKE2 create/edit flow via :show-toc="true". |
| shell/components/TableOfContents/index.vue | New ToC UI for rendering a nested list of located accordions. |
| shell/components/TableOfContents/composables.ts | New composables to register components and build an ordered tree + scrolling behavior. |
| shell/components/CruResource.vue | Adds showToc prop, renders TableOfContents, and introduces grid-based layout for content + ToC. |
| shell/assets/translations/en-us.yaml | Adds i18n string for “Jump to…”. |
| shell/assets/styles/global/_layout.scss | Adds global scroll-behavior: smooth. |
| pkg/rancher-components/src/components/Form/Radio/RadioGroup.vue | Sets component name to support name-based discovery/pattern matching. |
| pkg/rancher-components/src/components/Accordion/Accordion.vue | Registers accordions into the summary system and adds a scrollTo() helper. |
| pkg/imported/components/CruImported.vue | Enables ToC and adds temporary/demo accordions + controls (TODO-marked). |
| pkg/eks/components/CruEKS.vue | Enables ToC rendering on the EKS create/edit flow via :show-toc="true". |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
eb8b1da to
7360a0d
Compare
7360a0d to
227370f
Compare
f8814f5 to
6fa67a9
Compare
|
This is ready for review now, bearing in mind I've left some test components in CruImported |
|
Sorry for the comment spam if it reached you. I was trying an AI tool and it submitted all the comments before I could review them. |
f644b48 to
99fa746
Compare
56b9dbd to
b9c8f22
Compare
codyrancher
left a comment
There was a problem hiding this comment.
Only a couple comments.
We can wait to see what @rak-phillip has to say to my one comment. And the other is really just optional.
| import { computed, inject, provide, type Ref } from 'vue'; | ||
| import RcButton from '@components/RcButton/RcButton.vue'; | ||
| import RcIcon from '@components/RcIcon/RcIcon.vue'; | ||
| import { useInSummary } from '@shell/components/TableOfContents/composables'; |
There was a problem hiding this comment.
I see one of two options here depending on intention:
- If we want to expose ToC to all consumers of Rancher Components I have a feeling we should the entire implementation into Rancher Components.
- We only want to expose ToC to shell, in that case I think we should wrap RcSection in a shell component such as TableOfContentsSection which uses RcSection and ToC composables to make the new component.
Any thoughts on this @rak-phillip ?
There was a problem hiding this comment.
I agree with both of your positions. I think that the question that we need answered if is "do we consider ToC to be a core building block?"; if we do, then we need to sync with @oboc-sts to ensure that the toc is detailed in the design system. We'll also need to ensure that the ToC is properly documented in the storybook.
Based on what I'm seeing, I think the answer is yes - we might want this implemented in Rancher Components.
There was a problem hiding this comment.
In an effort to try and unblock this pr since it's definitely the reviewing that has held this one up. @oboc-sts, is the ToC component something we're planning to use in extensions or outside of rancher?
If so, can we put the component it in the design system?
There was a problem hiding this comment.
Per slack discussion it sounds like we dont want extension developers/people outside our team relying on this feature just yet.
There was a problem hiding this comment.
I just saw the message. Given that I'm okay to approve. I think we may want to move where the composable is used but I don't think we need to hold this up anymore for that.
| locatedComponents.value = buildTree([], parent?.vnode) || []; | ||
| }; | ||
|
|
||
| const debouncedLocateRegisteredComponents = debounce(locateRegisteredComponents); |
There was a problem hiding this comment.
debounce(locateRegisteredComponents) without a wait argument defaults to 0 ms, so it only defers to the next task.
If the intent is to batch rapid mount/unmount cycles (e.g. a v-for list updating), we may want a short delay.
If 0 ms is intentional (just deduplicate synchronous calls) I think we might want a comment for the intention.
There was a problem hiding this comment.
Fair callout, it does look odd without an explanation. I added this debounce because every component using the summary composable triggers this when rendered, so when a form with 9 accordions is loaded its called 9 times immediately.
I threw a console log in locateRegisteredComponents just to verify and calling debounce without a wait is sufficient to reduce this to one call when forms are initially rendered.
I added a comment to clarify its purpose.
| }, | ||
|
|
||
| setup(props) { | ||
| const instance = getCurrentInstance(); |
There was a problem hiding this comment.
getCurrentInstance usage is strongly discourage; it is an internal Vue implementation detail and is not subject to semver constraints, meaning that this can break on us without warning on a version bump. Have we exhausted all other approaches at this point?
There was a problem hiding this comment.
I've removed all uses of getCurrentInstance - more details in comments below
| const el = instance?.proxy?.$el as HTMLElement | undefined; | ||
|
|
||
| el?.scrollIntoView(true); |
There was a problem hiding this comment.
Can we use template refs instead of getCurrentInstance() to manage the scrolling?1
Footnotes
There was a problem hiding this comment.
Yes good call -- done
| setup(props) { | ||
| const instance = getCurrentInstance(); | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const t = (instance?.proxy as any)?.$store?.getters?.['i18n/t'] as ((key: string) => string) | undefined; |
There was a problem hiding this comment.
We have an established pattern for working with i18n in the composition API:
import { useStore } from 'vuex';
import { useI18n } from '@shell/composables/useI18n';
const store = useStore();
const { t } = useI18n(store);
I think that we should prefer this approach instead.
There was a problem hiding this comment.
You are quite right. I've updated all references to t to use this method instead.
| const instance = getCurrentInstance(); | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const t = (instance?.proxy as any)?.$store?.getters?.['i18n/t'] as ((key: string) => string) | undefined; | ||
| const label = computed(() => props.titleKey && typeof t === 'function' ? t(props.titleKey) : props.title); |
There was a problem hiding this comment.
Out of scope for this change - I think we should consider deprecating the titleKey pattern. Callers should be able to provide a valid title without Rancher Components requiring a dependency on the store and the i18n implementation in order to perform a key lookup. This current approach tightly couples components to the Dashboard implementation.
FYI @codyrancher
| import { computed, inject, provide, type Ref } from 'vue'; | ||
| import RcButton from '@components/RcButton/RcButton.vue'; | ||
| import RcIcon from '@components/RcIcon/RcIcon.vue'; | ||
| import { useInSummary } from '@shell/components/TableOfContents/composables'; |
There was a problem hiding this comment.
I agree with both of your positions. I think that the question that we need answered if is "do we consider ToC to be a core building block?"; if we do, then we need to sync with @oboc-sts to ensure that the toc is detailed in the design system. We'll also need to ensure that the ToC is properly documented in the storybook.
Based on what I'm seeing, I think the answer is yes - we might want this implemented in Rancher Components.
| const instance = getCurrentInstance() as any; // avoid TS error TS2339 | ||
| const t = instance?.proxy?.$store?.getters?.['i18n/t'] as ((key: string) => string) | undefined; | ||
| const component = ref<SummaryComponent | null>(null); |
There was a problem hiding this comment.
Prefer useStore()/useI18n()
There was a problem hiding this comment.
t is no longer needed in the composable as components are expected to provide a label
| const scrollTo = options?.scrollTo ?? (() => { | ||
| const el = instance?.proxy?.$el as ElementWithSummaryID | null | undefined; | ||
|
|
||
| (el as HTMLElement | undefined)?.scrollIntoView(true); | ||
| }); |
There was a problem hiding this comment.
nit: Do we need the fallback at this point? It looks like all of the current callers supply their own scrollTo method.
There was a problem hiding this comment.
I've removed this fallback in favor of making scrollTo a required argument. This, combined with making label and a property pointing to a vue template ref required, allowed me to remove the call to getCurrentInstance and also simplify this composable a fair bit.
The tradeoff is that components need to provide more arguments to register with the summary, but this seems like a good tradeoff to me. It avoids using getCurrentInstance and gives summarized components a lot more control over how they are summarized.
| component.value = new Proxy({} as SummaryComponent, { | ||
| get(_target, key: string) { | ||
| if (key === 'summary') { | ||
| return summary; | ||
| } | ||
| if (key === 'summaryID') { | ||
| return summaryID; | ||
| } | ||
| if (exposed && key in exposed) { | ||
| const val = exposed[key]; | ||
|
|
||
| return val && typeof val === 'object' && 'value' in val ? (val as { value: unknown }).value : val; | ||
| } | ||
|
|
||
| return (proxy as Record<string, unknown> | null | undefined)?.[key]; | ||
| } | ||
| }); |
There was a problem hiding this comment.
I don't think that I 100% understand why we currently need to perform this action. Is this for fallback behavior?
There was a problem hiding this comment.
This is no longer needed, since components must now provide all properties used by the summary when registering with the summary. But if you're curious, the intent was to allow registerComponents to treat compnents defined with the options api (everything exposed on proxy) the same as components defined with the composition api (everything private by default; select properties exposed on exposed)
| showAsForm: this.$route.query[AS] !== _YAML, | ||
| tocContainerHeight: 0, | ||
| mainLayoutEl: null, | ||
| throttledComputeTocContainerHeight: throttle(this.computeTocContainerHeight, 20), |
There was a problem hiding this comment.
I don't think the data prop is the correct location for including throttle. Maybe the created() or mounted() hook would be more appropriate?
See our epic on addressing issues with complex data props for more context on why we want to keep data lean: #11807
There was a problem hiding this comment.
Good call; I've moved the throttle call to the created hook
…he toc when less than our laptop width
7fbce65 to
06157b1
Compare
| banner.checkNotExists(); | ||
|
|
||
| // update the Global Permissions | ||
| userCreate.selectCheckbox('User-Base').set(); |
There was a problem hiding this comment.
It looks like this test is failing because this checkbox is hidden by the intentionally-introduced error banner. I can't discern any visual or behavioral differences in this area of the UI with this PR; I do not think this test failure is indicative of an actual unintended change observable by users.
Watching test runs, it appears that on the master branch, cypress is able to click a checkbox that is not visible:
Looks like this in this PR:
I think it is a fluke that it passed before and correct for the test to fail if trying to interact with an element that is hidden by another element...?
I updated the test to close the error banner before clicking the checkbox. In doing so I noticed the banner was not close-able and had to fix the CruResource props in the users page. I can move this work into a separate PR that blocks this PR if this is too much scope creep
Summary
Fixes #16118
Occurred changes and/or fixed issues
This PR adds a table of contents that works with the Accordion and RcSection components. It uses composables that may be re-used to create summaries of any vue components (eg all our input components).
Technical notes summary
This PR includes a new vue component, 2 new composables, and changes to CruResource, Accordion, and RcSection
TableOfContents.vue
Defines how to display a list of accordions provided by prop, focused on display logic
useFormSummaryanduseInSummaryThe
useInSummarycomposable makes components register themselves with the nearest ancestor component usinguseFormSummary. The only requirement to use this composable is that components must have anameproperty. Components are registered when mounted in the DOM: components hidden with v-show (eg components within a collapsed Accordion) are included in the summary. Components hidden with v-if will not be (until revealed by changing v-if)The
useFormSummarybuilds a tree of components registered by theuseInSummarymixin. It determines relative position of components by navigating the virtual DOM. The composable exposes a method to compute a reactive list of a subset of the components registered with the summary, filtered by name, or all components if no name pattern is provided (this is intended to facilitate implementation of other form summary elements at the same level as the table of contents)CruResource.vue
New prop added, showToc, to determine when to show the table of contents. In theory all it takes to add the table to existing forms that use Accordions is setting that prop true on the relevant instance of CruResource. The css was updated to accommodate TableOfContents.vue
Accordion and RcSection
These components have minimal updates to use
useInSummary.Areas or cases that should be tested
Screenshot/Video
Screen.Recording.2026-03-26.at.1.49.01.PM.mov
Checklist
Admin,Standard UserandUser Base