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

[NO-MERGE] DLT-1689 add overlay scrollbars example #330

Closed
wants to merge 8 commits into from

Conversation

ninamarina
Copy link
Contributor

@ninamarina ninamarina commented May 24, 2024

Custom scrollbars POC

Jira: DLT-1689

Adds the dependency Overlay Scrollbars to Dialtone to see how it works and what functionality it allows.

I created and exported a new directive DtScrollbarsDirective, that can be used similar to the DtTooltipDirective, just adding v-dt-scrollbars to the component that would normally have overflow: auto to allow scroll.

The Vue wrapper that this library offers is only available for Vue 3, so it's not an option to use that for now.

Notice that when there are listeners to the scroll event in the element where we are adding the custom scrollbars, some changes would have to be made (see Emoji Picker).

I tested in:

  • Message input
  • Leftbar (Ubervoice)
  • Emoji Picker
  • Conversation View (failed)

Message Input ✅

With custom scrollbars Native scrollbars
custom-scroll-mi native-scroll-mi

Leftbar ✅

With custom scrollbars Native scrollbars
custom-scroll-leftbar native-scroll-leftbar

Check the code for Ubervoice here.

Emoji Picker ✅

With custom scrollbars Native scrollbars
emoji-custom ep-native

Some changes were needed to make it work with the emoji picker, because the overlay scrollbars library adds some new elements to the DOM, and this component was using refs and attaching event handlers to the scrollable element.
When using this directive, the scrollable element is the second child of the element you attach the custom scrollbar to.

Conversation View ❌

When adding v-dt-scrollbars directive in the InfiniteList the render gets stuck in an infinite loop, so the conversation is never rendered. I tried debugging this with no success, so this one might be a component where we can't add this directive for now.

Advantages of using Overlays Scrollbars over native scrollbars

  • Allows custom styling and ensures that the look and feel will be the same across browsers and platforms. Everything can be customizable: the size, color, border, transitions, etc.
  • Allows overlay scrollbars (in fact there is no option for not overlay scrollbars)
  • Offer a lot of options to customize them, like when they should appear (on scroll, on hover or all the time)
  • It would be very simple for developers to use them in simple components (below there's a note on the Conversation View component*)

Disadvantages

  • The DOM structure ends up a little different than what you see in the template, because 4 divs are added inside the element to which the scrollbars are attached to. This might make it more difficult to use with components that add event listeners to the scrollable element.

Previous
image

Without the directive, the element with id d-emoji-picker-list is the scrollable element

With custom scrollbars
image

The element with id d-emoji-picker-list now has four divs inside:

  • One with the class "os-size-observer""
  • The second one is the actual scrollable element
  • The horizontal scrollbar
  • The vertical scrollbar

Accessibility

When focus is within the viewport controlled by a scrollbar, the Up Arrow and Down Arrow (or Left Arrow and Right Arrow for a horizontal scroll bar) should move the scroll bar thumb proportionally. Additionally, the Page Up, Page Down, Space, and Shift + Space keys must move the content and the scroll thumb the height (or width) of the viewport for each key press until the bottom or top (or left or right) of the content is in view.

  • Aria role: scrollbar

Because native scroll bar styling has historically been limited, you may come across a scrollbar implemented in JavaScript that you need to support and make fully accessible. For this, you can use the scrollbar role to inform assistive technologies that a UI control is an interactive scrollbar.

I don't think this role is added, and I didn't find a way to add it so this might be a problem for screen readers.

Next Steps

  • Test in different browsers and platforms, and different inputs, like touch on a mobile device
  • Decide if the overlay is the style we want for the scrollbar
  • Decide if we want to go with this solution even if it can't be used in every component

@ninamarina ninamarina force-pushed the poc/custom-scrollbar branch 2 times, most recently from 036b572 to 7228dff Compare May 27, 2024 21:22
@ninamarina ninamarina marked this pull request as ready for review May 30, 2024 18:53
@ninamarina ninamarina requested a review from fede-dp May 30, 2024 18:55
@ninamarina ninamarina marked this pull request as draft May 31, 2024 00:24
@ninamarina ninamarina marked this pull request as ready for review May 31, 2024 14:59
@braddialpad
Copy link
Contributor

Are we able to show the scrollbar on mouseover of the scrollable region rather than when you actually start scrolling? I think this would make it a bit more obvious that the region is actually scrollable.

@ninamarina
Copy link
Contributor Author

Are we able to show the scrollbar on mouseover of the scrollable region rather than when you actually start scrolling? I think this would make it a bit more obvious that the region is actually scrollable.

Yes, that is posible by setting the option scrollbars.autoHide to leave:

image

@braddialpad
Copy link
Contributor

Disadvantages
It's adding an external dependency for something that exists natively (although we don't have full control of the styles).

Not really a problem at all, we've investigated native functionality before and found it was actually impossible to do what we needed to do.

Doesn't work as expected with components that listen to the scroll event, like the Emoji Picker an the Conversation View.

We can certainly release this initially the way it is, however it would be nice if we had sort of alternative way to properly use events. Would it work if we had a directive as well as a component like we do for tooltip? The conversation view is a place where we would like to use this scrollbar if possible. (i know it's a mess)

@ninamarina
Copy link
Contributor Author

ninamarina commented May 31, 2024

Not really a problem at all, we've investigated native functionality before and found it was actually impossible to do what we needed to do.

Yes, I meant the functionality, not the styles.

We can certainly release this initially the way it is, however it would be nice if we had sort of alternative way to properly use events. Would it work if we had a directive as well as a component like we do for tooltip? The conversation view is a place where we would like to use this scrollbar if possible. (i know it's a mess)

I would need more time to explore this possibility with a Scrollbar component, I wanted to first get some feedback to decide if it's worth to explore that. But yes, I could certainly continue with the investigation to explore that possibility.
Of course, the downside of this, it's that the code for those components (like Conversation View and Emoji Picker) will end up being more complex than it is now.

Copy link

github-actions bot commented Jun 3, 2024

Please add either the visual-test-ready or no-visual-test label to this PR depending on whether you want to run visual tests or not.
It is recommended to run visual tests if your PR changes any UI. ‼️

Copy link

github-actions bot commented Jun 3, 2024

✔️ Deploy previews ready!
😎 Dialtone preview: https://dialtone.dialpad.com/deploy-previews/pr-330/
😎 Dialtone-vue 2 preview: https://dialtone.dialpad.com/vue/deploy-previews/pr-330/
😎 Dialtone-vue 3 the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-330/

Copy link
Contributor

@fede-dp fede-dp left a comment

Choose a reason for hiding this comment

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

looks nice!

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.

I'm okay with this for now, maybe we can find a solution to the scroll events issue in the future.

@ninamarina
Copy link
Contributor Author

The implementation was merged here: #391

@ninamarina ninamarina closed this Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants