-
Notifications
You must be signed in to change notification settings - Fork 334
16118 table of contents #16383
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
base: master
Are you sure you want to change the base?
16118 table of contents #16383
Changes from all commits
b51be91
0d4e6db
bb1387d
4b77a4c
baf97eb
5ab4894
62dcece
cf22f1c
369a921
561232f
9c0fb68
585c30b
ce13f2b
cfb0665
f8581d7
1bf9723
307abba
c4887a0
be25df3
9329d6e
668af5f
05f1602
93a1494
1f47b7c
f1dd38b
852118a
2e62135
cc3b6cb
38c750e
8c89665
71bc685
c2cf17d
7c24d74
b4a9342
f801b59
ed7ef43
d1c52a8
921d32a
e831585
25f244c
618a175
289c406
88de97f
9d134a7
ed0d0e3
1e331eb
7c3d98b
65eee4e
01bb494
c73c8d4
0857065
03a2b34
6df8d34
ed4317b
d431e55
b98a53e
06157b1
1c83aaf
3ec7cdc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,14 @@ | ||
| <script lang="ts"> | ||
| import { defineComponent } from 'vue'; | ||
| import { mapGetters } from 'vuex'; | ||
| import { | ||
| computed, defineComponent, nextTick, ref, useTemplateRef | ||
| } from 'vue'; | ||
| import { mapGetters, useStore } from 'vuex'; | ||
| import { useInSummary } from '@shell/components/TableOfContents/composables'; | ||
| import { useI18n } from '@shell/composables/useI18n'; | ||
|
|
||
| export default defineComponent({ | ||
| name: 'Accordion', | ||
|
|
||
| props: { | ||
| title: { | ||
| type: String, | ||
|
|
@@ -20,22 +26,59 @@ export default defineComponent({ | |
| } | ||
| }, | ||
|
|
||
| setup(props) { | ||
| const store = useStore(); | ||
| const { t } = useI18n(store); | ||
| const label = computed(() => props.titleKey && typeof t === 'function' ? t(props.titleKey) : props.title); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of scope for this change - I think we should consider deprecating the FYI @codyrancher |
||
|
|
||
| const isOpen = ref(props.openInitially); | ||
| const accordionSummarizedContainer = useTemplateRef<HTMLElement>('accordion-summarized-container'); | ||
|
|
||
| const scrollTo = () => { | ||
| isOpen.value = true; | ||
| nextTick(() => { | ||
| accordionSummarizedContainer.value?.scrollIntoView(); | ||
| }); | ||
| }; | ||
|
|
||
| const { summary } = useInSummary({ | ||
| scrollTo, | ||
| label, | ||
| elementRef: accordionSummarizedContainer, | ||
| }); | ||
|
|
||
| return { | ||
| summary, | ||
| isOpen, | ||
| scrollTo, | ||
| }; | ||
| }, | ||
|
|
||
| data() { | ||
| return { isOpen: this.openInitially }; | ||
| return {}; | ||
| }, | ||
|
|
||
| computed: { ...mapGetters({ t: 'i18n/t' }) }, | ||
| computed: { | ||
| ...mapGetters({ t: 'i18n/t' }), | ||
|
|
||
| displayTitle() { | ||
| return this.titleKey ? this.t(this.titleKey) : this.title; | ||
| }, | ||
| }, | ||
|
|
||
| methods: { | ||
| toggle() { | ||
| this.isOpen = !this.isOpen; | ||
| } | ||
| } | ||
| }, | ||
| }, | ||
| }); | ||
| </script> | ||
|
|
||
| <template> | ||
| <div class="accordion-container"> | ||
| <div | ||
| ref="accordion-summarized-container" | ||
| class="accordion-container" | ||
| > | ||
| <div | ||
| class="accordion-header" | ||
| data-testid="accordion-header" | ||
|
|
@@ -51,7 +94,7 @@ export default defineComponent({ | |
| data-testid="accordion-title-slot-content" | ||
| class="mb-0" | ||
| > | ||
| {{ titleKey ? t(titleKey) : title }} | ||
| {{ displayTitle }} | ||
| </h2> | ||
| </slot> | ||
| </div> | ||
|
|
@@ -67,7 +110,8 @@ export default defineComponent({ | |
|
|
||
| <style lang="scss" scoped> | ||
| .accordion-container { | ||
| border: 1px solid var(--border) | ||
| border: 1px solid var(--border); | ||
| border-radius: var(--border-radius); | ||
| } | ||
| .accordion-header { | ||
| padding: 16px 16px 16px 11px; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,9 +33,12 @@ | |
| * <p>Section content here</p> | ||
| * </RcSection> | ||
| */ | ||
| import { computed, inject, provide, type Ref } from 'vue'; | ||
| import { | ||
| computed, inject, provide, useTemplateRef, 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'; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see one of two options here depending on intention:
Any thoughts on this @rak-phillip ?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per slack discussion it sounds like we dont want extension developers/people outside our team relying on this feature just yet.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| import type { RcSectionProps, SectionBackground } from './types'; | ||
|
|
||
| const RC_SECTION_BG_KEY = 'rc-section-background'; | ||
|
|
@@ -58,6 +61,24 @@ provide(RC_SECTION_BG_KEY, resolvedBackground); | |
|
|
||
| const expanded = defineModel<boolean>('expanded', { default: true }); | ||
|
|
||
| // Expose summary, name, and a display label on the component public instance so | ||
| // TOC discovery can access component | ||
| const displayTitle = computed(() => props.title); | ||
|
|
||
| const name = 'RcSection'; | ||
|
|
||
| // Register this section in form summary/table-of-contents context (if provided) | ||
| const sectionRef = useTemplateRef<HTMLElement>('rc-section-summarized-container'); | ||
| const { summary } = useInSummary({ | ||
| label: displayTitle, | ||
| scrollTo: () => sectionRef.value?.scrollIntoView(true), | ||
| elementRef: sectionRef, | ||
| }); | ||
|
|
||
| defineExpose({ | ||
| summary, displayTitle, name | ||
| }); | ||
|
|
||
| const hasHeader = computed(() => { | ||
| return props.mode === 'with-header'; | ||
| }); | ||
|
|
@@ -84,7 +105,10 @@ function toggle() { | |
| </script> | ||
|
|
||
| <template> | ||
| <div :class="sectionClass"> | ||
| <div | ||
| ref="rc-section-summarized-container" | ||
| :class="sectionClass" | ||
| > | ||
| <div | ||
| v-if="hasHeader" | ||
| class="section-header" | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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 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