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

Alert Dialog should use Dialog #3860

Open
1 of 3 tasks
cogwizzle opened this issue Apr 12, 2024 · 3 comments
Open
1 of 3 tasks

Alert Dialog should use Dialog #3860

cogwizzle opened this issue Apr 12, 2024 · 3 comments
Labels
Area: Components Related to the component library (core) of this system Status: WIP This PR is still a work-in-progress. No need to review. Type: Enhancement Changes that don't affect the meaning of the code

Comments

@cogwizzle
Copy link

Description

When I use the alert dialog component, I expect that it will utilize the built in dialog element, instead it is using a bunch of divs in order to create the a dialog.

Link to Reproduction

https://paste.twilio.design/components/alert-dialog

Steps to reproduce

  1. Go to 'https://paste.twilio.design/components/alert-dialog#default-alert-dialog'
  2. Click on 'Open Alert Dialog'.
  3. Inspect Element.

Paste Core Version

latest

Browser

Google Chrome 123.0.6312.124

Operating System

  • macOS
  • Windows
  • Linux

Additional Information

Using the semantic elements moving forward will improve the accessibility of the applications that use this library. Potentially we should use a div if dialog does not exist, but if it does we should use dialog.

@cogwizzle cogwizzle changed the title Alert Dialog Alert Dialog should use Dialog Apr 12, 2024
@dosubot dosubot bot added Area: Components Related to the component library (core) of this system Status: WIP This PR is still a work-in-progress. No need to review. Type: Enhancement Changes that don't affect the meaning of the code labels Apr 12, 2024
@TheSisb
Copy link
Collaborator

TheSisb commented Apr 12, 2024

Hi @cogwizzle,

Thank you for the suggestion!

First an explanation: our Modal component is currently powered by https://reach.tech/dialog/, which claims to be "the accessible foundation of your React-based design system". We used this library because our Modal component was created roughly 4 years ago. In that time, browser vendors have added the native Dialog element. This new element received baseline support in browsers within the past couple years. So we aren't using the native Dialog element because it was released after we needed it.

Our current implementation has served us well and we haven't received any complaints about its usage, performance, or accessibility. Now, that isn't to say that we don't think it's worth exploring migrating to the native element. However, we must be careful with any changes in our system to avoid potential breaking changes. A swap of the underlying technology could lead to unintended consequences to our downstream teams. For that reason we have to consider a broad range of factors when considering work like this, such as the benefits of the change, how reliable the solution is (do each browsers implement quirks?), feature parity, guardrails to prevent doing too much with it, the impact-to-effort ratio, where this work falls on our priority list and roadmap, and so on.

Are you testing this component with NVDA or Jaws? Is there anything you noticed was missing or improperly implemented? Providing that kind of information can help us make better decisions when we rebalance our backlog.

Thanks!

@TheSisb TheSisb mentioned this issue Apr 12, 2024
3 tasks
@cogwizzle
Copy link
Author

Are you testing this component with NVDA or Jaws? Is there anything you noticed was missing or improperly implemented? Providing that kind of information can help us make better decisions when we rebalance our backlog.

I am not testing with either tool. I happened to be utilizing Paste in a product (I'm a Twillion) and I noticed some odd accessibility issues first with Combobox. Then I kind of just started browsing the library and started noticing other potential problems. I was basing my recommendations on my previous experiences with div based modals / popups.

For that reason we have to consider a broad range of factors when considering work like this, such as the benefits of the change, how reliable the solution is (do each browsers implement quirks?), feature parity, guardrails to prevent doing too much with it, the impact-to-effort ratio, where this work falls on our priority list and roadmap, and so on.

I agree with this assessment 100%. We should consider the implications of browsers within a reasonable range. I understand the div technique was the way to go up until recently within the last few years. I do believe that the transition will provide a more seamless maintenance long term for the implementors of Paste as well as provide the most accessible experience.

Finally my reasoning for bringing it up was that I wanted to push stuff onto the radar of the Paste team rather than remaining silent about these things. I understand we can't always do everything that may need to happen as time is finite. Ultimately though I don't have visibility into the current workload of the Paste team so I couldn't know if it could be able to be prioritized. I do ultimately think small tweaks like this have a wide reach since this is fundamentally a foundational library used in many places. I hope that makes sense from a consumer's perspective.

@serifluous
Copy link
Member

(Shadi can totally continue replying from an eng perspective but just wanted to chime in too)

Finally my reasoning for bringing it up was that I wanted to push stuff onto the radar of the Paste team rather than remaining silent about these things. I understand we can't always do everything that may need to happen as time is finite.

We toooootally appreciate this. Hope you don't see our responses as pushback for the sake of pushback! We just want to set expectations with everyone about what we can address and improve in a realistic amount of time, and why it might end up falling where it does in our priority list (so they can better plan their roadmaps too). Appreciate the suggestions and shared motivation of trying to make all our customer experiences better 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Components Related to the component library (core) of this system Status: WIP This PR is still a work-in-progress. No need to review. Type: Enhancement Changes that don't affect the meaning of the code
Projects
None yet
Development

No branches or pull requests

3 participants