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

Dialog Component #1214

Merged
merged 31 commits into from
Aug 31, 2022
Merged

Dialog Component #1214

merged 31 commits into from
Aug 31, 2022

Conversation

keithamus
Copy link
Member

@keithamus keithamus commented Jul 22, 2022

This is the initial implementation of the Primer::Alpha::Dialog component.

This takes some of the code we already have in github/github from Primer::Experimental::Dialog, however I've dropped some properties for it to avoid pulling in configurations which we may want to later remove - to avoid breaking changes. So for example this doesn't implement helpers around <include-fragment> (those helpers cause unnecessary coupling between the two components, and can be abstracted away into different helpers), nor does this support forms (we need to more carefully consider the interplay between dialogs and forms, especially with regard to how native <dialog> will play a role here).

This takes the modal-dialog web component in its entirety from github/github. While this is a re-implementation of native <dialog>, and it probably makes sense to simply implement <dialog> instead, unfortunately we've noticed some areas where <dialog> isn't quite suitable: the support is only there in newer browsers, and some browsers have buggy implementations, and we need some time to look over the polyfill to make sure it handles what we want and the trade-offs are worth it. While we haven't had time to assess this yet, the <modal-dialog> component is ready to go, and I believe a later refactor from <modal-dialog> to <dialog> should be possible without any breaking changes.

Another area we should look into as a follow-up to this initial implementation is how we can leverage the overlay component (#1143) to simplify some areas of the dialog work.

@changeset-bot
Copy link

changeset-bot bot commented Jul 22, 2022

🦋 Changeset detected

Latest commit: 7ad9af3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/view-components Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@primer-css primer-css temporarily deployed to github-pages July 22, 2022 17:54 Inactive
@primer-css primer-css temporarily deployed to github-pages July 25, 2022 09:15 Inactive
@keithamus keithamus force-pushed the initial-dialog-implementation branch 2 times, most recently from ef2f621 to 9f3e2bb Compare July 25, 2022 13:12
@primer-css primer-css temporarily deployed to github-pages July 25, 2022 13:17 Inactive
@keithamus keithamus force-pushed the initial-dialog-implementation branch from 376d524 to 8f0d4aa Compare July 25, 2022 15:54
@primer-css primer-css temporarily deployed to github-pages July 25, 2022 16:00 Inactive
@keithamus keithamus force-pushed the initial-dialog-implementation branch from 94bfc44 to 20cd643 Compare July 25, 2022 16:37
@primer-css primer-css temporarily deployed to github-pages July 25, 2022 16:42 Inactive
@owenniblock owenniblock self-requested a review July 26, 2022 11:44
@keithamus keithamus force-pushed the initial-dialog-implementation branch 2 times, most recently from 9489471 to 7f5fd84 Compare July 26, 2022 11:51
@primer-css primer-css temporarily deployed to github-pages July 26, 2022 11:56 Inactive
@keithamus keithamus force-pushed the initial-dialog-implementation branch 3 times, most recently from 7d688af to c582828 Compare July 27, 2022 10:25
@primer-css primer-css temporarily deployed to github-pages July 27, 2022 10:31 Inactive
@keithamus keithamus force-pushed the initial-dialog-implementation branch from a945fa5 to d574ac6 Compare July 27, 2022 16:47
app/components/primer/alpha/dialog.rb Show resolved Hide resolved
app/components/primer/alpha/dialog.rb Outdated Show resolved Hide resolved
app/components/primer/alpha/dialog.rb Outdated Show resolved Hide resolved
@keithamus keithamus force-pushed the initial-dialog-implementation branch from d574ac6 to fe6e1c5 Compare July 28, 2022 16:46
app/components/primer/alpha/dialog.rb Outdated Show resolved Hide resolved
app/components/primer/alpha/dialog.rb Show resolved Hide resolved
app/components/primer/alpha/dialog.rb Outdated Show resolved Hide resolved
ModalDialogElement: typeof ModalDialogElement
}
interface HTMLElementTagNameMap {
'modal-dialog': ModalDialogElement

Choose a reason for hiding this comment

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

Sidenote: I've been wondering whether we should normalize Primer custom elements with a primer prefix. In this case, we don't use the term "modal" to refer to dialogs except in this custom component due to the hyphen requirement. Another example is the tool-tip element.

Maybe primer-dialog, primer-tooltip, primer-split-page-layout, etc?

Related: ADR on spelling of component names

Copy link
Contributor

Choose a reason for hiding this comment

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

We also presumably need to use a different name because we'll be running the Experimental modal-dialog still until we've worked out how we're handling all of the bits of functionality that were added to that version. I like primer-dialog.

@keithamus keithamus force-pushed the initial-dialog-implementation branch 3 times, most recently from 0cb5186 to 9469ea0 Compare August 18, 2022 16:44
@keithamus keithamus changed the title initial dialog implementation Dialog Component Aug 18, 2022
@khiga8
Copy link
Contributor

khiga8 commented Aug 27, 2022

👋 I'm not sure familiar with the state of dialog is currently but wanted to bring this audit comment from @jscholes to your attention in case it hasn't been surfaced yet.

@primer-css primer-css temporarily deployed to github-pages August 31, 2022 16:16 Inactive
@keithamus keithamus temporarily deployed to github-pages August 31, 2022 16:26 Inactive
@jonrohan jonrohan temporarily deployed to github-pages August 31, 2022 16:33 Inactive
@keithamus keithamus temporarily deployed to github-pages August 31, 2022 17:24 Inactive
@jonrohan jonrohan temporarily deployed to github-pages August 31, 2022 20:48 Inactive
@keithamus keithamus merged commit 33949aa into main Aug 31, 2022
@keithamus keithamus deleted the initial-dialog-implementation branch August 31, 2022 21:05
@primer-css primer-css mentioned this pull request Aug 31, 2022
@langermank
Copy link
Contributor

👋 Documenting some tips for rolling this out to dotcom (replacing Experimental::Dialog)

Experimental param Alpha param note
footer_content_align deprecated remove param and do visual check
header_variant deprecated remove param and do visual check
motion deprecated this has a default value now
description subtitle name change

The height and width params are combined into a new size option. I'll try and map out how these swaps might look, but visual testing is likely required.

width new size
:small :small
:medium :small
:large :medium
:xlarge :large
:xxlarge :xlarge

The size param handles height and width. If there's a case where an Experimental::Dialog has a width of :large and a height set to anything above :large, set the size to medium_portrait.

For Dialogs wrapped in <form> tags, work with @keithamus on how to structure the Alpha Dialog.

@keithamus keithamus mentioned this pull request Sep 23, 2022
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.

10 participants