PatternFly 5 Modal by dabeng · Pull Request #33 · patternfly-webcomponents/patternfly-webcomponents

dabeng · 2017-03-22T13:02:45Z

@bleathem @priley86 @dlabrecq Would you like to review this PR again?

FYI, currently Firefox doesn't support attributeChange callback of Custom Elements v1. Please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=1334051 for more details.

priley86 · 2017-03-22T15:18:02Z

wow! this is a great first start @dabeng.

Can you do me a favor and go ahead and expand the iframe size in app.html for this PR? Ideally we can change it to something like:

    function resizeIframe(obj) {
      obj.style.height = obj.contentWindow.document.body.scrollHeight + 200 + 'px';

This is a really promising start. We will definitely need some tests (please see my last merged PR for notes on how to debug these locally with Karma/Phantom).

Also, maybe a modal with a form like the one here would be nice?

You may want to ask @dgutride and @dlabrecq for any advice here too.

priley86 · 2017-03-22T15:14:21Z

+   * An instance of the element is created or upgraded
+   * We build up the complete DOM tree of web components
+   */
+  constructor () {


We aren't quite here yet with v1 constructors. Can you use the v0 attachedCallback for now? Would like to change all these new components in a new PR for #27

got it. no question

dabeng · 2017-03-24T09:42:55Z

Hi @priley86 , I have added unit testing and replaced custom elements v1 with v0. Please review agagin 😊

priley86 · 2017-03-25T13:32:43Z

wow! this is awesome @dabeng !!! 😈

Thanks for making the v0 change. Looks good!

I have one more additional request after reviewing a bit more. Can we pretty please with sugar on top change the <div modalPart="header"> to <pf-modal-header class="modal-header"> (a new simple CE)? The main idea here is we are trying to make the DOM more expressive for the consumer and eliminate necessary divs where possible.

I think it's fine to keep the Bootstrap template separate like you have done, we would just need to query this new pf-modal-header custom element in the createdCallback of pf-modal instead and then copy its content into your div class="modal-header in the rendered template. We could also allow the user to override the class here too w/ the pf-modal-header CE (i.e. we could default its rendered class to modal-header if nothing is provided, but if a class is provided from the user, we could override with that class). You may find it easier to change the template structure though and just render the modal-header div inside the new CE. Either way should work!

I would suggest the same patterns for pf-modal-body and pf-modal-footer.

I know this is a little bit of refactor, but take your time. You're doing an awesome job with this already!! 👍

priley86 · 2017-03-25T13:35:58Z

also - you may want to check your tests in build w/ Phantom too? Travis seems to be upset again.

priley86 · 2017-03-25T13:16:01Z

+   */
+  _afterShowModal () {
+    this._element.focus();
+    this._element.dispatchEvent(new CustomEvent('modalShown', { 'detail': { 'relatedTarget': this._target }}));


can we change the event name to shown.bs.modal to match the Bootstrap API?

priley86 · 2017-03-25T13:16:46Z

+      }
+    }
+
+    this._element.dispatchEvent(new CustomEvent('modalHide', {}));


same thing here.. can we go with hidden.bs.modal instead?

priley86 · 2017-03-25T13:17:08Z

+   * @public
+   */
+  show () {
+    this._element.dispatchEvent(new CustomEvent('modalShow', { 'detail': { 'relatedTarget': this._target }}));


Can we go with show.bs.modal?

What about "show.pf.modal"?

After all, bs is bootstrap keyword.

You raise a great point. We have been using the same events as Bootstrap before (so we'd have to adopt this convention globally), but I'm not opposed to this.

I will raise a question on this to the mailing list this week. Feel free to pledge support for this...

Hi @priley86 , "show.bs.modal" is the special event namespace of jquery style. Are you sure to introduce it into patternfly web components?

priley86 · 2017-03-25T13:17:39Z

+   * @private
+   */
+  _hideModal () {
+    pfUtil.removeClass(document.body, 'modal-open');


hide.bs.modal

dabeng · 2017-03-26T01:15:43Z

After running "gulp test", all unit tests succeed. I don't know why travis failed.

recrsn · 2017-03-26T01:46:18Z

@dabeng, Travis will fail if any of these tasks fails: lint, test, doc
Try running all the three gulp tasks.
And there also could be runtime errors in Travis like, out of memory.

recrsn · 2017-03-26T01:52:11Z

-      'src/**/*.spec.js'
+      'src/**/*.spec.js',
+      'dist/css/patternfly.css',
+      'dist/css/patternfly-webcomponents.css'


Please do not include CSS files here. PhantomJS goes out of memory when you try to load CSS and WebFonts in Travis.

Where should I place these two css file references?

Do not include them at all. You probably need CSS for transitions, instead of depending upon the events, manually call the handlers

dabeng · 2017-03-26T01:53:28Z

Hi @priley86 , I thought about custom element pf-modal-header, but I found that it violated the aims of Custom Element -- reusability. Except for being included in pf-modal, are there other scenarios where pf-modal-header can apply in?

In fact, what we need is just a container which contains custom content for header(body or footer). We don't even care what container element. <div>, <section>, even <a> are all right. It's really a fuss to make a standalone custom element just for a wrapper element.

recrsn · 2017-03-26T01:54:56Z

+    pfUtil.addClass(this._mask, 'in');
+    pfUtil.reflow(this._element);
+    pfUtil.addClass(this._element, 'in');
+    pfUtil.once(this._dialog, 'transitionend', this._afterShowModal, this);


You will need to manually call afterShowModal in your tests.

I did so already

Then removing your CSS files will probably make the travis errors go away.

To exactly simulate Travis, make an ubuntu 14.04 lxc container with 1.5gb memory allocated to it and run the tests inside it 😈

priley86 · 2017-03-27T14:39:10Z

@dabeng I think the main idea with pf-modal-header, etc. is to make the DOM more expressive. There is a really good article on this here.

For too long we have encouraged "div" soup and this project aims to avoid that. Ideally, we can still support the "div" scenarios by using common attributes or properties as selectors, but what I am suggesting is that divs are no longer necessary in many cases. Really they should only be needed when the consumer is extending the template with a LOT of custom style (typically they would just needed to change a class on the pf-modal-header though). But if we use a common selector in pf-modal though, the CE pf-modal-header along with a div are both valid, however one is obviously more expressive.

Does this sort of make more sense why I am suggesting this now? I see your point about reusability of child CEs, however I much prefer the "sugar" of having a more expressive syntax (and typically these children CEs are pretty simple to implement). Any more thoughts here?

dlabrecq · 2017-03-27T17:30:49Z

+   * @private
+   */
+  _keydownHandler (e) {
+    let isEscape = (e.key && e.key === 'Escape') || (e.keyIdentifier && e.keyIdentifier === 'U+001B') || (e.keyCode && e.keyCode === 27);


Need to support IE and Edge. We will need to test if keyCode exists, for example:

let keycode = event.keyCode ? event.keyCode : event.which;

I have tested this PR on IE10 or Edge. It worked well.

Besides, there are too many code snippets of patternfly-webcomponents waiting for cross-browser checkout. keydown event of "Escape" key is just the tip of the iceberg. That's why I wanna introduce jquery as a common module at the beginning.

jQuery is just a Module providing us with the syntactic sugar of basic DOM operation. It's essentially different with angular, react, vuejs. It's not essentially different with vanilla javascript if we just make use of its core DOM functionalities of jquery.

There is a reason that Bootstrap 4 doesn't give up jQuery

There are cases where event.keyCode does not exist.

And many Angular developers won't use the components because of jQuery -- it's a non-starter for some people.

Hi @dabeng, i think it is important to keep in mind that our core two target frameworks initially will be the Angular JS and React JS frameworks. These are still the most common frameworks in use at Red Hat today. What @dlabrecq is saying is certainly true about many of these communities avoiding the use of Jquery. It adds additional API overhead (where frameworks often come in solving the same problems) and typically is not as clean as fit as framework native components or now, web components. You might have seen this link on our PF Showcase page about how to avoid jquery:
http://codeblog.cz/vanilla/overview.html#prerequisities

With the amount of existing components implementing complex behaviors with Jquery (such as our treeview), we obviously have a long ways to go to remove it and reimplement these components. But we've learned a lot from some of the communites below on how to do this...

http://www.vanillalist.com/
http://thednp.github.io/bootstrap.native/

Hi @dlabrecq , I have added event.which testing.

dlabrecq · 2017-03-27T17:39:21Z

+    <hr>
+
+    <button class="btn btn-default" id="btn-toggle-modal">Launch demo modal</button>
+    <pf-modal targetSelector="#btn-toggle-modal" backdrop keyboard>


I'm concerned that developers must provide all this raw HTML themselves. This particular HTML is basically a cut and paste from the PF core test page. It appears that the component is used mainly to open/close the modal?

I believe the component should provide the header, close button, etc. As an example, consider the modal component from Angular Patternfly. All this HTML is output by the component -- more complicated layouts are defined using transclusion. Thus, the APF modal example is much more simplified and looks more like this:

<div pf-about-modal is-open="isOpen" on-close="onClose()"
additional-info="additionalInfo" product-info="productInfo" title="title"
copyright="copyright" img-alt="imgAlt" img-src="proxy.php?url=https%3A%2F%2Fgithub.com%2Fpatternfly-webcomponents%2Fpatternfly-webcomponents%2Fpull%2FimgSrc"></div>

I think "provide all this raw HTML themselves" is just what users want most from pf-modal because children custom elements(pf-modal-header, pf-modal-body, pf-modal-footer) provide users with the maximal free space to construct their own internal structure of pf-modal 😊

I disagree. The Angular Patternfly modal was created because developers don't want to have to find an example and provide the raw HTML themselves. Otherwise, there is little value in creating a component.

could we not provide different variations of the pf-modal CE, such as an pf-about-modal which inherits or reimplements this pf-modal CE? @dlabrecq @dabeng

i too was thinking about this with something such as a "USA State dropdown" or a "USA Zip code select" component? These types of common UI components may become a possibility with web components...

@priley86 @dlabrecq @bleathem The core functionality of pf-modal is providing a togglable dialog. It's just a dialog wrapper with show/hide effect and doesn't care what's internal structure of dialog. Can we come to an agreement about this point ?

I think the value of web components should not include completing stuff outside its duty. e.g. custom content or layout.

<pf-modal> <pf-modal-header>custom content</pf-modal-header> <pf-modal-body>custom content</pf-modal-body> <pf-modal-footer>custom content</pf-modal-footer> <pf-modal>

let modal = new PfModal({
headerInfo: {…},
bodyInfo: {…},
footerInfo: {…},
otherOptions: {…}
});
document.body.appendChild(modal);

There gotta be a better one.

dlabrecq · 2017-03-27T17:50:15Z

+    this._element.setAttribute('tabindex', -1);
+    let headerPart = this._element.querySelector('[modalPart="header"]').innerHTML;
+    let bodyPart = this._element.querySelector('[modalPart="body"]').innerHTML;
+    let footerPart = this._element.querySelector('[modalPart="footer"]').innerHTML;


Just a nit, but vars should appear top of the function.

dlabrecq · 2017-03-27T17:59:36Z

+   * @public
+   */
+  show () {
+    this._element.dispatchEvent(new CustomEvent('show.bs.modal', { 'detail': { 'relatedTarget': this._target }}));


We should discuss, but I don't necessarily agree we should use Bootstrap event names? For example, we generate a tooltip event, but we don't use shown.bs.tooltip as the event name.

We have many other events that don't necessarily match up with Bootstrap. I'm thinking we need to decide on common naming convention for the sake of consistency -- it's getting messy.

Here a just a few of our existing event names:

show.bs.dropdown/hide.bs.dropdown
initialized
itemClicked
tooltipOpened/tooltipClosed
thresholdSet
tabChanged

+1 for standardizing these. Thus the question 😉

dlabrecq · 2017-03-27T18:01:20Z

+   */
+  _afterShowModal () {
+    this._element.focus();
+    this._element.dispatchEvent(new CustomEvent('shown.bs.modal', { 'detail': { 'relatedTarget': this._target }}));


We should document exactly what events, names, and properties are generated. If not for this private function, at least for the public ones. And perhaps also at the top of the component's JsDoc to make it clear what public events are available?

dabeng · 2017-03-28T11:00:58Z

Hi @priley86 , I have added children CEs(pf-modal-header, pf-modal-body and pf-modal-footer)

priley86 · 2017-03-28T19:54:54Z

thanks @dabeng , i think this looks good, but will wait for others to weigh in on this.

Have we decided on an event naming convention @bleathem ? do we prefer show.pf.hidden,etc. ? Maybe we can get another PR going to clean this up (I'm partially responsible for this inconsistency!) ;0

created #35 for this...

dabeng · 2017-04-05T08:16:57Z

@priley86 @dlabrecq @bleathem , there is one other point I would like to discuss with you.

Some web components are suitable for declaration with html, like pf-tabs. Some are suitable for instantiation with javascript. And others can be created by any of two above ways.

Here, I think it's a better way to create pf-modal with declaration pattern because the way can facilitate users to construct the internal content of pf-modal's every sections.

dlabrecq · 2017-04-05T15:12:53Z

There was a discussion during the last UXD/UI Developer meeting and believe the consensus is that we want to provide a pattern's default behavior first, but allow developers to override if necessary.

For example, the pf-modal-header component can provide the close button and title layout by default. If a developer wishes to override that, they can replace it using transclusion via the body of pf-modal-header.

Likewise, the pf-modal-footer component should provide the save and cancel buttons by default and then allow developers to override if necessary. The pf-modal-body component is fine as is.

dabeng · 2017-04-05T23:12:02Z

Cool. This consensus is a great improvement. New commit is coming 😊

dabeng · 2017-04-06T10:36:10Z

I have added "override" feature for pf-modal-header and pf-modal-footer, hope current commit can archive your requirements @priley86 @dlabrecq @bleathem

priley86 · 2017-04-06T12:38:19Z

nice improvement @dabeng, works for me. Can you do me a huge favor and make paths relative, and then test your test page with Travis? I left instructions in #38 for how to do this... please just post your test page link here once done...

priley86 · 2017-04-06T12:36:09Z

+<html>
+<head>
+    <title>pf-tooltip example</title>
+    <link rel="stylesheet" href="/dist/css/patternfly.css">


you'll want to make all of these dist/css/ paths relative after the recent update. i.e. ../../dist/css/patternfly.css. Check the other components for an example on this...

priley86 · 2017-04-06T12:36:22Z

+<!doctype html>
+<html>
+<head>
+    <title>pf-tooltip example</title>


should be: pf-modal example

Sorry for my carelessness

Test page of pf-modal -- https://rawgit.com/dabeng/patternfly-webcomponents/modal-dist/src/pf-modal/index.html

dlabrecq · 2017-04-06T14:52:40Z

+ * @shown.bs.modal It's fired when the modal has been made visible to the user (will wait for CSS transitions to complete). If caused by a click,
+ * the clicked element is available as the relatedTarget property of the event.detail
+ * @hide.bs.modal It's fired immediately when the hide instance method has been called
+ * @hidden.bs.modal It's fired when the modal has finished being hidden from the user (will wait for CSS transitions to complete)


We need to change these events from Bootstrap specific to the new convention the community agreed upon.

dlabrecq · 2017-04-06T14:54:53Z

+<body>
+<div class="container">
+    <div class="page-header">
+      <h1>Modals</h1>


Can the title be an attribute?

Sorry @dlabrecq , I can't understand this requirement. Could you explain it in more details?

Please disregard that comment. I thought I was looking at the pf-modal-header. It's difficult to read when GitHub injects all these comments between lines of code.

dlabrecq · 2017-04-06T14:59:13Z

+          </div>
+        </form>
+      </pf-modal-body>
+      <pf-modal-footer></pf-modal-footer>


Are there events generated for the close (in header), save, and cancel buttons? It would be good if this example showed how to capture a save event.

If you look at our Angular Patternfly examples for reference, that uses a textarea to output/confirm which event/button was just clicked.

@dlabrecq , I can't find your mentioned textarea on http://www.patternfly.org/angular-patternfly/#/api/patternfly.modals.directive:pfAboutModal. Could you give me the right link?

I wasn't specifically speaking about the modal example, but something more like listView, toolbar, etc. Click on the buttons in the example below and you'll see the interaction.

http://www.patternfly.org/angular-patternfly/#/api/patternfly.views.directive:pfListView

OK. No question.

dlabrecq · 2017-04-06T15:02:41Z

+      expect(callback).toHaveBeenCalled();
+    });
+  });
+


Now that we're providing default cancel, save, and close buttons, we should test the events here. For example, we can test if clicking the close button hides the modal. Users should be able to receive an event for save, cancel, etc.

dabeng · 2017-04-07T03:28:36Z

@priley86 , Test page of pf-modal -- https://rawgit.com/dabeng/patternfly-webcomponents/modal-dist/src/pf-modal/index.html

priley86 · 2017-04-07T17:31:39Z

Thanks @dabeng. The modal looks to work, but it seems that your branch needs rebasing? If I go here for instance:
https://rawgit.com/dabeng/patternfly-webcomponents/modal-dist/index.html

It does not load the same files as it does in master. It should look like this one...with your modal added:
https://rawgit.com/patternfly-webcomponents/patternfly-webcomponents/master-dist/index.html

dabeng · 2017-04-07T17:37:07Z

I'll check this issue ASAP after I append enough unit testing.

dabeng · 2017-04-07T17:39:33Z

@priley86 , Is pf.modal.show final custom event namespace convention?

priley86 · 2017-04-13T13:19:51Z

@dabeng wow! great job...the tests are very helpful and it looks like you've captured all of the events.

priley86 · 2017-04-13T13:20:17Z

@dlabrecq any more thoughts on these updated unit tests?

priley86 · 2017-04-13T13:47:46Z

One other thing here... after looking at setInnerHtml() further, you are accepting html (which I think is fine), but it doesn't look like you are re-initializing the component? I am just noting that slight difference w/ the pf-tooltip which calls init() after handleContentChanged fires in the attachedCallback.

I'm not sure if it matters or not with your CE though...just want to be sure we can support the case of something like this (rendered in a framework):

<pf-modal>
<pf-modal-header>header</pf-modal-header>
<pf-modal-body> <p> Greetings earthlings, we come in {frameworkModel.salutation} </p> </pf-modal-body>
<pf-modal-footer>footer</pf-modal-footer>
</pf-modal>

Just want to be sure that when the pf-modal-body content changes via the framework binding (which occurs outside the CE's lifecycle) the CE content updates. If it's not updating the content, I'd suggest allowing the outside framework shim to fire handleContentChanged to notify the CE to reinitialize.

Can we just verify this? You could also just write some html/js to test it like:

<pf-modal id='pf-modal'>
<pf-modal-header>header</pf-modal-header>
<pf-modal-body> <p> Greetings earthlings, we come in <span id='saluation'>peace</span> </p> </pf-modal-body>
<pf-modal-footer>footer</pf-modal-footer>
</pf-modal>

js:

var pfModal = document.getElementById('pf-modal');
document. getElementById('#salutation').textContent = 'search of web components';
pfModal.dispatchEvent(new CustomEvent('handleContentChanged', {}));

Does the content of the pf-modal-body then change?

dabeng · 2017-04-14T07:42:02Z

@priley86 , the following code snippets can change the content of pf-modal-body. No problem.

var pfModal = document.getElementById('pf-modal');
document. getElementById('#salutation').textContent = 'search of web components';

Additionally, according to @dlabrecq's recommendation:

The best we can do is document this as the recommended way to set transcluded content for Patternfly components. If users choose not to follow the docs, we cannot stop them.

In pf-modal, we just allow uses to fire handleContentChanged event by calling setInnerHtml() method.

priley86 · 2017-04-17T12:59:21Z

@dabeng do you mind creating a plunker demonstrating this (like I have done with pf-tooltip)? I think the next step is to test this out with Angular / React bindings...

This will also help us out write any shims that might be necessary here.

dabeng · 2017-04-18T03:48:43Z

@priley86 @dlabrecq hope you like the Demo

dabeng · 2017-04-24T13:13:39Z

@priley86
This is Using pf-modal in ReacJS
And this is Using pf-modal in Angular

priley86 · 2017-04-24T18:14:14Z

thanks very much @dabeng for providing this... these templates are great and we can certainly use them in the future!

For React, I am really hoping to support something like this:
http://codepen.io/priley86/pen/qmaxwd

This gives an example of how content can change via props/bindings. This will definitely happen in real app scenarios where you are passing dynamic content via props/state into your modal (and it will have to update). I guess my hope is that we support this kind of behavior via Mutation Observer or providing some event to tell the WC to reload itself...we can mask this in the React shim though...

Thanks again for researching, this is great!!!

priley86 · 2017-04-25T15:33:25Z

in looking at this again, @dabeng, another option may be to "shim" the modal-header, modal-body, and modal-footer and just write the content as this.props.children. If this works, we should still get bindings, but you will have to test that it works with the WC... My guess is it is the same challenge, but I'm not sure...

dabeng · 2017-04-26T00:33:30Z

Sorry @priley86 , I couldn't get your points, can you explain in more detail?

write the content as this.props.children

dabeng · 2017-04-27T05:52:36Z

As for pf-modal demo under anguarl 2 context. I think it's a good timing when we replace custom elements v0 with v1.

priley86 · 2017-04-27T13:05:19Z

Sorry @dabeng, it is hard to give a full explanation without a complete example... I think the simple use case is still valid (and a good first step to test). Can you make this WC support the codepen I forked?
http://codepen.io/priley86/pen/qmaxwd

The issue is we need to make the WC respond to changes in state/props being passed down into the template of the modal. When I click "Change field one", I'm updating state (and props coming down), but the content label does not change. The content in the modal body will be assumed dynamic by the consumer (angular or react).

The other idea I'm having is to create React shims (which internally render pf-modal-mody) and inside it's body we render this.props.children. That content is then dynamic based on what is transcluded by the consumer....

Long story short, if you can fix the use case for the codepen I've added (and reload the content either via Mutation Observer or an event), then that use case should work as well...(we'd just replicate the solution)...

Hope this helps...

dabeng · 2017-04-28T01:45:34Z

Hi @priley86 , I have to use dangerousSetInnerHTML() method in order to render HTML string as real HTML in JSX. Maybe there are other better solutions. Please review again Demo

As for "create React shims", @dlabrecq has denied the use of mutation

observer:Mutation observers have their own performance issues, which is why we're avoiding them here.

So I moved to use setInnerHTML() method and dispatch pf-modal.changed event.

priley86 · 2017-04-28T18:55:04Z

@dabeng have you seen @Agathver's latest plnkr?
https://plnkr.co/edit/cGrMMm?p=preview

My vote is to do something similar here. This is part of the issue with setting nested content as a string. That is not the typical scenario by any React / Angular consumer I don't think. I.e. most React components I work with wrap nested components as this.props.children (and render them inside). Would like to stay consistent w/ this...

Can you take a look at this one and see what you think? We are still having an open conversation on this, so appreciate any feedback...

dabeng · 2017-04-29T13:44:36Z

Hi @priley86 , you mean I should replace setInnerHTML() with mutation observer?

dabeng · 2017-05-03T09:02:14Z

@priley86 For the last few days, I tried introduced MutationObserver object into the pf-modal. However, I found that it's not a good idea to build up web components based on mutation observer mechanism because it will result in a huge of redundant and low-performance code snippets as well as poor readability and maintainability. Specially, it's a nightmare for complicated and nested web component to work with mutations.

recrsn · 2017-05-03T09:23:07Z

@dabeng The entire Polymer project is heavily based on MutationObservers, and they have been stabilized for quite a time, so it can be safe to assume the performance issues of MutationObservers have been alleviated by now.

http://stackoverflow.com/questions/31659567/performance-of-mutationobserver-to-detect-nodes-in-entire-dom

Sure there is some performance hits (~3%), but the convenience it gives, is worth it, and in many cases it won't be the performance bottleneck either.

priley86 · 2017-05-03T14:23:22Z

@dabeng @Agathver my vote is to try to abstract the Observers away as best we can for now (can we possibly write a helper to contain that logic?). Either way, same performance issue is there for Angular...and I'm not sure of another way to fire the handleContentChanged event once Angular has finished rendering a binding. Using an event and firing once the way we do w/ React will be more performant.

also, kudos to @cdcabrera for the abstraction idea...could this go in pf-utils.js?

Long term, I think we look to slotchanged offered by the Shadow DOM slots.
https://github.com/webcomponents/gold-standard/wiki/Content-Changes

dabeng · 2017-05-08T08:34:41Z

@priley86 I have made the pf-modal respond to changes in state/props being passed down into the template of the modal. Could you the demos on the top of this page again?

priley86 · 2017-05-09T01:16:56Z

@dabeng great job... Really like the examples across Angular/React you did! These are really useful for future components and show how flexible web components can be...

PF-modal is definitely a more complex component which we can all learn from. I think the examples adequately show conditional rendering and you did a great job w/ this. The API is a bit confusing to me from the examples, so I definitely think writing a really doc page for this will be useful (clearly denoting different WC attributes/intended usages).

My previous plnkr appears to be updating correctly now, but styling is off and looks like there is some event handler error in console. Any idea? It doesn't appear we have a MutationObserver now, but the pf-modal-body seems to be rendered separately (works). So it may not be needed...
http://codepen.io/priley86/pen/qmaxwd

@cdcabrera @dlabrecq @bleathem any more thoughts before we move forward here? I think next step is to clearly document the API...would <3 a PR here documenting the PF Modal next.

Let's try to keep the examples clear and concise, and use case focused now that we know it supports the complex patterns...

dabeng · 2017-05-09T05:55:31Z

@priley86 I added assistant elements(pf-modal-dialog and pf-modal-content), please see this demo
http://codepen.io/dabeng/pen/GmyxWy

priley86 · 2017-05-12T12:57:34Z

awesome @dabeng ! the new codepen looks great. thanks so much...

Let me know once you have some API docs ready for showcase site or if you want any help ;) I will definitely review those w/ you.

dabeng · 2017-06-16T00:12:06Z

@priley86 ，I have upgraded pf-modal to v1, please review Demos again.

priley86 · 2017-06-16T17:52:30Z

looks good to me @dabeng ... the v1 examples appear to be working correctly in everything other than Firefox for me (as you noted). These also appear to be different issues than we ever had w/ v0. Is there an upstream webcomponent polyfill issue we can track (thanks for finding the Firefox bug)? You're definitely the farthest along w/ v1 research so thanks a lot for pushing this forward...

Any short term workarounds for this?

The only other thing I noted was a minor difference in the Cancel/Save buttons and our PF Core test page. The button heights are different for some reason. Can you double check that we don't need to extend this modal with any new styles?

https://rawgit.com/patternfly/patternfly/master-dist/dist/tests/modals.html

priley86 · 2017-06-16T17:55:55Z

@dabeng I guess attributeChangedCallback is currently in development for FF...

http://caniuse.com/#feat=custom-elementsv1

cdcabrera · 2017-06-21T19:48:52Z

@dlabrecq @priley86 @dabeng merged this in, open issues accordingly

dabeng mentioned this pull request Mar 22, 2017

pf-modal development #29

Closed

priley86 assigned dgutride and dlabrecq Mar 22, 2017

priley86 suggested changes Mar 22, 2017

View reviewed changes

priley86 suggested changes Mar 25, 2017

View reviewed changes

recrsn reviewed Mar 26, 2017

View reviewed changes

dlabrecq requested changes Mar 27, 2017

View reviewed changes

priley86 mentioned this pull request Mar 28, 2017

clean up custom event names #35

Closed

priley86 suggested changes Apr 6, 2017

View reviewed changes

dlabrecq requested changes Apr 6, 2017

View reviewed changes

PatternFly 5 Modal.

7390c3b

priley86 approved these changes Jun 21, 2017

View reviewed changes

priley86 mentioned this pull request Jun 21, 2017

Firefox v1 support - attributeChangedCallback #47

Open

cdcabrera merged commit 2cfe1e7 into patternfly-webcomponents:master Jun 21, 2017

Uh oh!

Conversation

dabeng commented Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

priley86 commented Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dabeng commented Mar 24, 2017

Uh oh!

priley86 commented Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

priley86 commented Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dabeng Mar 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dabeng commented Mar 26, 2017

Uh oh!

recrsn commented Mar 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dabeng commented Mar 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

recrsn Mar 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priley86 commented Mar 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlabrecq Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dabeng commented Mar 22, 2017 •

edited

Loading

priley86 commented Mar 22, 2017 •

edited

Loading

priley86 commented Mar 25, 2017 •

edited

Loading

priley86 commented Mar 25, 2017 •

edited

Loading

dabeng Mar 26, 2017 •

edited

Loading

dabeng commented Mar 26, 2017 •

edited

Loading

recrsn Mar 26, 2017 •

edited

Loading

dlabrecq Mar 28, 2017 •

edited

Loading

dlabrecq Mar 27, 2017 •

edited

Loading

priley86 Mar 28, 2017 •

edited

Loading

dabeng Mar 29, 2017 •

edited

Loading

dlabrecq Mar 27, 2017 •

edited

Loading