Don't create a close button automatically

[shawnbot]

Removes the automatically added close button in favor or allowing implementors to supply their own. Originally it did this:

Fixes #10 by checking for whether a close button ([data-close-dialog]) exists in the DOM before adding one automatically. There's a simple test for it, which passes! 🎉

But after some discussion, we agreed that nixing the close button altogether was the best move.

I've also introduced CLOSE_ATTR, CLOSE_SELECTOR, and INPUT_SELECTOR constants to keep things dry, and exposed them as static members of the (now default-exported) custom element class, which means that you can do this in other modules, e.g. in React:

import DetailsDialog from 'details-dialog-element'
import React from 'react'

const closeProps = {
  [DetailsDialog.CLOSE_ATTR]: true
}

export default ({children, ...rest}) => (
  <details-dialog {...rest}>
    {children}
    <button {...closeProps}>Close me!</button>
  </details-dialog>
)

[dgraham]

What do you think about removing the default close button from the element's responsibilities and leave it to the host application to provide one? That would leave the custom element responsible for its open states, form reset on close, and tab behavior.

Because the element is responsible for open and close it makes sense that it includes an Escape key handler and click handler on the modal backdrop to close itself. Including a visual close button feels more like the responsibility of the app.

The data-close-dialog attribute is a lightweight way to participate in the element's state that we could still support.

Including the close button feels a little prescriptive when multiple presentations are possible. For example, here's this custom element with close button icon in the corner.

And here's another possible handling of a text labeled close button positioned at the bottom.

Another sign of tension in these responsibilities is the inline octicon svg from Primer. Removing the default button would let the host app render icons from its preferred style library. We would use the element with Primer like this in the Rails app.

<details>
  <details-dialog>
    <button type="button" class="dd-close-button" aria-label="Close" data-close-dialog>
      <%= octicon("x") %>
    </button>
  </details-dialog>
</details>

The example React usage above could provide the close button from a library function or inline JSX.

[shawnbot]

I'm down with that, @dgraham. Another way we could handle it is to provide the button for screen readers only, so that it becomes purely an accessibility feature. In that case, maybe you would be able to opt out of it explicitly, e.g. with close-button="false" or something?

[muan]

🙌🏻 All very good and valid points. This was first added to replicate what facebox has.

Including a visual close button feels more like the responsibility of the app.

Agree. I'm for removing the close button altogether and let the user decide on what to do. This way we avoid having to assume style and pulling in octicon.

I think we can also get away from not providing a screen reader only button assuming Escape key is a common pattern for escaping dialogs.

[muan]

This is a great idea!

[shawnbot]

Do you all have a preference for this over exporting it at the module level?

[muan]

I prefer this.

[shawnbot]

Uhh also, why does this work when the selector is missing the ]?

details-dialog-element/test/test.js

Lines 48 to 53 in 0969b44

	const close = dialog.querySelector('[data-close-dialog')
	assert(!details.open)
	dialog.toggle(true)
	assert(details.open)
	close.click()
	assert(!details.open)

[shawnbot]

Okay, I've removed the automatically added close button and the corresponding CSS, and marked the failing tests as pending. I also updated the tests to use the constants, but I'm honestly not sure whether that's good practice or not. 😬 Ready for review?

[muan]

Looks good! Merge master for test fix in #12?

[muan]

I prefer this.