Nested submenus #12548
Nested submenus #12548
Conversation
vitvakatu
requested review from
jdunkerley,
radeusgd,
GregoryTravis,
AdRiley,
marthasharkey,
Frizi,
farmaazon and
kazcw
as code owners
🧪 Storybook is successfully deployed!📊 Dashboard:
|
github-actions
bot
added
the
-libs-API-change-Base
| defineExpose({ | ||
| isTargetOutside, | ||
| }) | ||
|
|
||
| export interface Submenu { | ||
| entries: ComputedRef<Entry[]> | ||
| relativeTo: HTMLElement | ||
| } | ||
|
|
||
| const submenu = ref<Submenu | null>(null) | ||
| const submenuEntries = computed(() => submenu.value?.entries ?? []) | ||
| const submenuRef = useTemplateRef('submenuRef') |
There was a problem hiding this comment.
I propose a bit of reordering here, for better reading: types at the top (after props maybe), then refs, then rest and expose at end (the last is not a rule per se, but a place I usually look for exposes).
There was a problem hiding this comment.
Did some reordering, trying to keep related parts together.
| <Teleport v-if="props.rootElement" :to="props.rootElement"> | ||
| <div ref="dropdownElement" :style="floatingStyles" class="SelectionSubmenu widgetOutOfLayout"> | ||
| <SizeTransition height :duration="100"> | ||
| <DropdownWidget |
There was a problem hiding this comment.
I just realized we have DropdownWidget and DropdownMenu which, of course, don't use DropdownWidget at all.
I think the difference should be documented. Or these two could be merged? But the latter shall be rather a separate task.
There was a problem hiding this comment.
Yeah… We have a couple of really confusing names here:
components/FloatingSelectionMenu.vue
components/GraphEditor/widgets/WidgetSelection
components/GraphEditor/widgets/WidgetSelection/SelectionSubmenu.vue
components/GraphEditor/widgets/WidgetSelection.vue
components/SelectionDropdown.vue
components/SelectionDropdownText.vue
components/SelectionMenu.vue
components/DropdownMenu.vue
components/SelectionDropdown.vue
components/SelectionDropdownText.vue
components/widgets/DropdownWidget.vue
There was a problem hiding this comment.
I’m not sure how to address it properly at the moment, we might need to do some renamings/refactorings, but I think it is out of scope of this PR. I will create an issue instead.
|
|
||
| /** | ||
| * Recursively find the nearest <li> element among ancestors of the event target. | ||
| * It is used to guarantee a stable target no matter the exact location of the click. | ||
| */ | ||
| function findTargetLiElement(target: EventTarget | null): HTMLElement | null { | ||
| if (!(target instanceof HTMLElement)) return null | ||
| let targetLiElement = target | ||
| while (!(targetLiElement instanceof HTMLLIElement)) { | ||
| targetLiElement = targetLiElement.parentElement as HTMLElement | ||
| } | ||
| return targetLiElement | ||
| } |
There was a problem hiding this comment.
Couldn't we just use event.currentTarget on li element?
If not, add explanation somewhere, why.
There was a problem hiding this comment.
Ah, yep, it will work.
| export function singleChoiceConfiguration(config: SingleChoice): OneOfFunctionCalls { | ||
| const possibleChoices = config.values.flatMap(flattenChoice) | ||
| return { | ||
| kind: 'OneOfFunctionCalls', | ||
| possibleFunctions: new Map( | ||
| config.values.map((value) => [value.value, functionCallConfiguration(value.parameters)]), | ||
| possibleChoices.map((choice) => [choice.value, functionCallConfiguration(choice.parameters)]), | ||
| ), | ||
| } | ||
| } | ||
|
|
||
| /** A configuration for the inner widget of a multiple-choice selection widget. */ | ||
| export function multipleChoiceConfiguration(config: MultipleChoice): SomeOfFunctionCalls { | ||
| const possibleChoices = config.values.flatMap(flattenChoice) | ||
| return { | ||
| kind: 'SomeOfFunctionCalls', | ||
| possibleFunctions: new Map( | ||
| config.values.map((value) => [value.value, functionCallConfiguration(value.parameters)]), | ||
| possibleChoices.map((choice) => [choice.value, functionCallConfiguration(choice.parameters)]), | ||
| ), |
There was a problem hiding this comment.
It would be supernice to have these method tested.
If they are (I couldn't find), then extend them with case with nested choices.
There was a problem hiding this comment.
Added some tests, although the implementation is very straightforward and I doubt anything will break here, but it can break in the usage place instead, and it is far more complicated to test.
in DropdownWidget.vue Also rename `target` → `htmlElement` in API
Pull Request Description
Closes #12182
To test, you can provide widget configuration with nested items, like the following:
nested-dropdown-menus.mp4
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.