Skip to content
This repository has been archived by the owner on Dec 15, 2023. It is now read-only.

feat: components tooltip and popover. absolute positioning (dt 40) #194

Merged

Conversation

nikitadialpad
Copy link
Contributor

@nikitadialpad nikitadialpad commented Nov 14, 2021

components tooltip and popover. absolute positioning using tippy js (dt 40)

๐Ÿ› ๏ธ Type Of Change

  • Fix
  • Feature
  • Refactoring
  • Documentation

๐Ÿ“– Description

The PR contains implementation of a tooltip using tippy headless, dialtone style and dialtone-vue components.
This implementation covers such drawbacks like rendering tooltip at the end of the body, dynamic positioning/"flipping" feature for tooltip and popover, hide on escape, modal prop was added to popover component interface.

As was investigated and discussed tippy headless is chosen as popover and tooltip engine library, because of that v-click-outside is removed, that part now covers by tippy.

๐Ÿ“ Checklist

  • I have updated library exports
  • I have reviewed my changes
  • I have added tests
  • I have added all relevant documentation
  • All tests are passing
  • All linters are passing
  • No accessibility issues reported
  • I have validated components with a screen reader
  • I have validated components keyboard navigation

๐Ÿ”ฎ Next Steps

๐Ÿ“ท Screenshots / GIFs

2021-11-15 17 00 53

2021-11-15 17 06 42

* DT 40 Absolute positioning for popovers & tooltips

* updates

* updates

* update tooltip

* update tooltip

* clean up

* refactoring, clean up

* resolve comments

* resolve comments
refactor first iteration stories

* update comments
* add documentation for Tippy tooltip

* update reactive "show"

* resolve comments, update eslint config because of an error

* update esplint json
* add documentation for Tippy tooltip

* update reactive "show"

* update tests

* update tests

* resolve comments

* resolve comments
* DT 40 Absolute positioning for popovers & tooltips

* updates

* updates

* update tooltip

* update tooltip

* clean up

* refactoring, clean up

* resolve comments

* popover implementation, decomposition tippy, clean up

* add animation, clean up

* clean up popover

* clean tooltip folder

* add import

* clean up, updates

* DT-40 popover component, refactoring after tooltip implementation

* resolve comments

* clean up

* resolve comments

* fix eslint

* leftover after merge

* leftover after merge
Update components DtLazyShow, popover, tooltip;
@nikitadialpad nikitadialpad self-assigned this Nov 14, 2021
@nikitadialpad
Copy link
Contributor Author

nikitadialpad commented Nov 15, 2021

@dbecherdialpad @braddialpad

there're some updates after merge implementation PRs to the branch:

  • added dt-lazy-show for tooltip after popover implementation
  • updated tests for tooltip after
  • added after-leave event to dt-lazy-show to fix incorrect opening and closing tooltip with custom triggers.
  • added modal prop for popover ( it's a description for that in the story). According to that change, it's a question
  • added flip feature for popover
  • updated documentation for popover
  • removed v-click-outside dependency, tippy manages that now.

Could you please confirm now that previous popover and tooltip implementation should be completely removed now?

<div
v-if="open && modal"
class="d-modal"
aria-hidden="false"
Copy link
Contributor Author

@nikitadialpad nikitadialpad Nov 15, 2021

Choose a reason for hiding this comment

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

usage d-modal will not show overlay with aria-hidden="true". is it need to update dialtone with some styles for popover with modal prop?

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed on slack, we decided to create a new class specifically for popover modal

@nikitadialpad nikitadialpad marked this pull request as ready for review November 15, 2021 14:52
โ€ฆ-positioning

# Conflicts:
#	components/popover/popover.stories.js
#	components/popover/popover_default.story.vue
Copy link
Contributor

@braddialpad braddialpad left a comment

Choose a reason for hiding this comment

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

We made a bunch of adjustments to our storybook prop/slot organization recently, which of course was not included in this tooltip since it was not in staging. Most of my comments are related to this.

components/tooltip/tooltip.test.js Outdated Show resolved Hide resolved
components/popover/popover.stories.js Outdated Show resolved Hide resolved
<div
v-if="open && modal"
class="d-modal"
aria-hidden="false"
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed on slack, we decided to create a new class specifically for popover modal

components/tooltip/tooltip_tippy.stories.js Outdated Show resolved Hide resolved
components/tooltip/tooltip_tippy.stories.js Outdated Show resolved Hide resolved
components/tooltip/tooltip_tippy.stories.js Outdated Show resolved Hide resolved
components/tooltip/tooltip_tippy.stories.js Outdated Show resolved Hide resolved
Copy link
Contributor

@braddialpad braddialpad left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple small things

components/lazy_show/lazy_show.vue Show resolved Hide resolved
components/modal/modal.vue Show resolved Hide resolved
components/popover/popover.vue Show resolved Hide resolved
components/tooltip/tooltip.test.js Outdated Show resolved Hide resolved
Copy link
Contributor

@braddialpad braddialpad left a comment

Choose a reason for hiding this comment

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

Thanks!

@braddialpad braddialpad merged commit dd9e44d into staging Nov 25, 2021
@braddialpad braddialpad deleted the components-Tooltip-Popover-DT-40-absolute-positioning branch November 25, 2021 17:06
nikitadialpad pushed a commit that referenced this pull request Nov 25, 2021
# [1.6.0](v1.5.0...v1.6.0) (2021-11-25)

### Bug Fixes

* add our alpha sort to list-item. Change slot types to VNode for consistiency ([#202](#202)) ([686b2b9](686b2b9))

### Features

* components tooltip and popover. absolute positioning (dt 40) ([#194](#194)) ([dd9e44d](dd9e44d)), closes [#131](#131) [#177](#177) [#178](#178) [#135](#135)
@github-actions
Copy link

๐ŸŽ‰ This PR is included in version 1.6.0 ๐ŸŽ‰

The release is available on GitHub release

Your semantic-release bot ๐Ÿ“ฆ๐Ÿš€

class="d-ps-relative dt-popover"
>
<component :is="elementType">
<dt-lazy-show
Copy link
Contributor

@dbecherdialpad dbecherdialpad Nov 30, 2021

Choose a reason for hiding this comment

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

@braddialpad @nikitadialpad couple suggestions from testing this with the emoji popover in the feed list (which was one of the drivers for doing this):

  1. The popover overlay should be at the end of the DOM as well, as a sibling of the popover itself. This way it properly overlays the content (see video example, where the feed actions disappear on hover and take the overlay with it)
  2. The overlay should have the option of being transparent. Arguably, this should be the default (every popover has a "transparent" overlay preventing you from interacting with the page behind it). In the case of the feed list, this is so you can't scroll the feed while the popover is open.
emoji.mov

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
3 participants