Skip to content
This repository was archived by the owner on May 1, 2025. It is now read-only.

Move Dropdown keydown listener to select element#438

Merged
lucaswerkmeister merged 2 commits intomasterfrom
dropdown
Jun 29, 2021
Merged

Move Dropdown keydown listener to select element#438
lucaswerkmeister merged 2 commits intomasterfrom
dropdown

Conversation

@lucaswerkmeister
Copy link
Copy Markdown
Member

This event listener shouldn’t be called for events bubbling up from the label (including any components in the suffix slot).

Bug: T277762


Tested with the following storybook patch:

diff --git a/vue-components/stories/Popover.stories.ts b/vue-components/stories/Popover.stories.ts
index 24ce522..941aaf4 100644
--- a/vue-components/stories/Popover.stories.ts
+++ b/vue-components/stories/Popover.stories.ts
@@ -1,5 +1,6 @@
 import Popover from '@/components/Popover';
 import Icon from '@/components/Icon';
+import Dropdown from '@/components/Dropdown';
 import Button from '@/components/Button';
 import { PopoverPositions } from '@/components/PopoverProps';
 import { Component } from 'vue';
@@ -115,3 +116,32 @@ export function all(): Component {
 all.parameters = {
     controls: { hideNoControlsWarning: true },
 };
+
+export function T277762(): Component {
+    return {
+        components: {
+            Button,
+            Dropdown,
+            Icon,
+            Popover,
+        },
+        template: `
+            <Dropdown
+                :menuItems="[{label: 'label', description: 'description'}]"
+                label="a dropdown"
+                style="margin: 5em;"
+            >
+                <template v-slot:suffix>
+                    <Popover>
+                        <template v-slot:target>
+                            <Button iconOnly>
+                                <Icon type="info-outlined" size="small" />
+                            </Button>
+                        </template>
+                      Popover content
+                    </Popover>
+                </template>
+            </Dropdown>
+        `,
+    };
+}

This event listener shouldn’t be called for events bubbling up from the
label (including any components in the suffix slot).

Bug: T277762
@github-actions
Copy link
Copy Markdown

@lucaswerkmeister
Copy link
Copy Markdown
Member Author

Alternative fix that I decided against:

diff --git a/vue-components/src/components/Popover.vue b/vue-components/src/components/Popover.vue
index 6f6a234..2bccf6c 100644
--- a/vue-components/src/components/Popover.vue
+++ b/vue-components/src/components/Popover.vue
@@ -6,7 +6,7 @@
 		@mouseleave="endHover"
 		v-detect-click-outside="clickOutsideHandler"
 	>
-		<span class="wikit-Popover__target" @click="onTargetClick">
+		<span class="wikit-Popover__target" @click="onTargetClick" @keydown="onTargetKeydown">
 			<!-- @slot Target should always be a button, as we will listen to its click and hover events -->
 			<slot name="target" />
 		</span>
@@ -99,6 +99,13 @@ export default Vue.extend( {
 		onTargetClick(): void {
 			this.changeContentVisibility( true );
 		},
+		onTargetKeydown( event: Event ): void {
+			if ( event.key === 'Enter' ) {
+				// visibility will be changed by associated click event,
+				// just stop this one from bubbling out of the component
+				event.stopPropagation();
+			}
+		},
 		startHover(): void {
 			if ( !this.reactToHover ) {
 				return;

Copy link
Copy Markdown
Contributor

@sai-san sai-san left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💯 I tested the changes locally and the fix seems to have corrected the unwanted beaviour. Thank you Lucas!

Copy link
Copy Markdown
Collaborator

@micgro42 micgro42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks too! 👍

@lucaswerkmeister lucaswerkmeister merged commit bf7b941 into master Jun 29, 2021
@lucaswerkmeister lucaswerkmeister deleted the dropdown branch June 29, 2021 09:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants