Component - Modal

[kahowell]

In our efforts to port the subscriptions cockpit UI from the cockpit repo, we have need of a modal dialog. It was relatively easy to simply copy the cockpit implementation into our repo and tweak (for now), but it'd be awesome if we could consume from patternfly-react instead.

See https://github.com/cockpit-project/cockpit/blob/c04c148225c59d2f053300d80b6b4731e421b8e6/pkg/lib/cockpit-components-dialog.jsx the cockpit implementation

See candlepin/subscription-manager#1716 for how we're integrating the cockpit-derived widget at the moment.

I'm not sure that the widget as currently written fits well with patternfly-react standards... there's some awkward DOM manipulation. I think a new, clean implementation might be a better fit; otherwise I'm happy to start a PR loosely based on the cockpit dialog implementation (after making sure cockpit devs are okay w/ that, etc). Thoughts?

[priley86]

@kahowell please feel free to start this. I agree with your assessment of the Cockpit implementation - it looks a bit dated now with the direct dom access / adding document event listeners. There are APIs for adding event listeners now in React (see the Notification List in this repo for example). Also the use of self should no longer be needed in es6 (as we have lexical scope with arrow functions now).

My guess is this can be simplified a lot using something like React Bootstrap's implementation here:
https://react-bootstrap.github.io/components.html#modals

Also, we will have to look into the accessibility of this component (ensuring we support escape key to close, tab selection, etc.)

Let me know if you have any questions or if i can help in any way!

[priley86]

FWIW, i am not tied to React Bootstrap implementations directly, but see them as good examples. We can choose to extend them (using inheritance), or reimplement them. Reimplementation may prove easier in the long term when we upgrade to Bootstrap4 though.

[priley86]

@kahowell have you started this? I may end up taking a stab at this alongside the Wizard in #64 just extending the React Bootstrap modal for now. I should have something up by end of day to get us started...

[kahowell]

@priley86, no, we ended up just using the cockpit dialog widget for now, but we'll happily swap it out when possible; once a PR is up, I'm happy to try it out and give feedback.

[priley86]

@kahowell awesome! I will report back soon...

[priley86]

@kahowell a preliminary PR is up... repurposing this issue in #68