Skip to content
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

[Tooltip] Complete API #231

Merged
merged 23 commits into from Jul 25, 2023
Merged

[Tooltip] Complete API #231

merged 23 commits into from Jul 25, 2023

Conversation

onmax
Copy link
Collaborator

@onmax onmax commented Jul 23, 2023

Description

Complete the Tooltip API.

Relevant changes:

  • All props for Tooltip* components with comments
  • I created ScreenReaderOnly component
  • Refactor useHoverDelay -> useHover
  • Removed TooltipPortal in favor of Teleport
  • Added comments to PopperContentProps
  • Radix React uses onEscapeKeyDown and onPointerDownOutside callback functions as props in TooltipContent. I think we decided that we instead we would emit events.

Regarding @Focus

At some point I think we have to update the Tooltip focus event. We don't really want to open it for every event, just for events triggered by keyboard.

I opened this vueuse/vueuse#3254 so we can use that composable or we just copy and paste that function into our project, as we want.

In Radix, for example, they just check if the event was triggered by a PointerDown, see Radix Code

Related Issue

#149

Motivation and Context

How Has This Been Tested?

Visually and I created some stories. Not sure how to do propper stories, that's why I did the minimum to test the props

Screenshots (if appropriate):

@onmax onmax changed the title [Tooltip] feat: Added props to TooltipArrow [Tooltip] Complete API Jul 23, 2023
onmax and others added 16 commits July 24, 2023 08:57
* [Accordion]feat: 2/3

* Added useSingleOrMultipleValue

* [Accordion]fix: Check if item is disabled onClick

* [useSingleOrMultipleValue] fix: use defaultValue from props

* [AccordionStory] fix: Use collapsible for single type

* [Accordion] fix: Remove unnecessary type in inject
* fix avatar rendering issue

* add nuxt playground to demo ssr

* fix requestAnimationFrame throwing error

* fix element selected even though undefined

* fix navigation menu not updating position for indicator correctly
* Fix : Alert Dialog handle click outside

* Fix : remove event handler
@onmax onmax marked this pull request as ready for review July 25, 2023 04:05
@onmax onmax requested a review from zernonia July 25, 2023 04:05
<slot />
<ScreenReaderOnly :id="injectedValue?.contentId" :ariaLabel="ariaLabel" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

We already have VisuallyHidden component, I think it's the same as this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just updated the code, I had to update VisuallyHidden component with a new slot.

In NavigationMenuTrigger, we have the following code:

<template v-if="open">
  <VisuallyHidden
    aria-hidden
    :tabIndex="0"
    :ref="setFocusProxyRef"
    @focus="handleVisuallyHiddenFocus"
  >
  </VisuallyHidden>
  <span :aria-owns="contentId" v-if="context?.viewport"></span>
</template>

But I don't fully understand this code. I would appreciate any feedback of my current implementation, which imitates Radix implementation.

packages/radix-vue/src/Tooltip/TooltipContent.vue Outdated Show resolved Hide resolved
packages/radix-vue/src/HoverCard/HoverCardTrigger.vue Outdated Show resolved Hide resolved
packages/radix-vue/src/Tooltip/TooltipTrigger.vue Outdated Show resolved Hide resolved
Copy link
Collaborator

@zernonia zernonia left a comment

Choose a reason for hiding this comment

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

lgtm!

@zernonia zernonia merged commit 65a9ff6 into main Jul 25, 2023
3 checks passed
@onmax onmax deleted the onmax/tooltip branch July 26, 2023 02:26
misbahansori added a commit to misbahansori/radix-vue that referenced this pull request Jul 26, 2023
* [Tooltip] feat: Added props to TooltipArrow

* useHoverDelay accept callbacks and options

* Updated useHover

* Updated PopperContent

* [Tooltip] Finished API

* [Accordion]feat: 2/3 (radix-vue#211)

* [Accordion]feat: 2/3

* Added useSingleOrMultipleValue

* [Accordion]fix: Check if item is disabled onClick

* [useSingleOrMultipleValue] fix: use defaultValue from props

* [AccordionStory] fix: Use collapsible for single type

* [Accordion] fix: Remove unnecessary type in inject

* [Bug] ssg/ssr not rendering correctly (radix-vue#233)

* fix avatar rendering issue

* add nuxt playground to demo ssr

* fix requestAnimationFrame throwing error

* fix element selected even though undefined

* fix navigation menu not updating position for indicator correctly

* chore: release v0.1.10

* [Tooltip]fix : tooltip open on keyboard focus (radix-vue#238)

* [Tooltip]fix: bind props values to data-side and data-align, remove unnecessary question mark (radix-vue#237)

* [Alert Dialog]fix : Alert Dialog handle click outside (radix-vue#236)

* Fix : Alert Dialog handle click outside

* Fix : remove event handler

* chore: release 0.1.11

* [Tooltip] feat: Removed TooltipPortal in favor of Teleport

* [Tooltip] feat: Use flg disable on hover

* Tooltip: Define emits event in TooltipContent

* Tooltip: Create stories folder

* Tooltip: Fixed content story

* Revert HoverCard changes

* Use VisuallyHidden component

* run lint

* fix triggerElement missing, test some story props

---------

Co-authored-by: zernonia <59365435+zernonia@users.noreply.github.com>
Co-authored-by: khairulhaaziq <rhycoz@yahoo.com>
Co-authored-by: Misbah Ansori <misbah.ansori24@gmail.com>
Co-authored-by: Artem Melnyk <51422045+MellKam@users.noreply.github.com>
Co-authored-by: zernonia <zernonia@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants