pf-accordion component by recrsn · Pull Request #30 · patternfly-webcomponents/patternfly-webcomponents

recrsn · 2017-03-13T17:35:40Z

This PR brings in the pf-accordion component, as mentioned in #25

priley86 · 2017-03-15T12:54:57Z

@Agathver awesome job with this! I am really impressed 😸

I tested cross browser and it worked as expected.

One functional thing I noticed. When one item is collapsed and we attempt to collapse a second item, the first item should close (like our test page here).

I would say next, can you add multiple accordions to your test page and just show that show(), hide() and toggle() are working?

Feel free to get going with tests when you have time! Once tests are done, we can also just double check the generated docs look OK. You can see an example of those here now.

Fantastic job with this @Agathver ! I really appreciate this!!!

recrsn · 2017-03-15T17:04:21Z

One functional thing I noticed. When one item is collapsed and we attempt to collapse a second item, the first item should close (like our test page here).

I've fixed it in my latest commit. However, I couldn't get the test case for this behavior done. pf-accordion, in its current form doesn't work well with JS generated content.

recrsn · 2017-04-03T20:24:53Z

I've fixed all the issues and implemented all the behavior of the PF accordion component.

~~Original Fixed Height PF Accordion, can only be initialized and the behavior may not be subsequently removed. (i.e, Accordion to Fixed Height Accordion conversion is one-time and irreversible)~~ Fixed this too.

priley86

Hey @Agathver, again great job with this!

As another step, can you please make the paths relative and test out building this branch in your fork? I left instructions on this in #38 . Let me know if you have any troubles...

Just want to ensure this looks properly and paths are correct once merged...

priley86 · 2017-04-06T12:56:45Z

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


can we make all paths relative? i.e.../../dist/css/patternfly.css

This will enable sharing this page in your fork. Check other components for examples...

priley86 · 2017-04-09T22:05:07Z

Hey @Agathver - your accordion here is looking good! (just sharing this link for other reviewers)
http://rawgit.com/Agathver/patternfly-webcomponents/pf-accordion-dist/app/app.html?dir=pf-accordion&file=index.html

I think a found a way to break it though ;)

Try these steps:

upon first load click "Collapsible Group Item Move pf-utilz.bar.chart to own repo., created generic pf doc page #1" (collapsed) in first accordion. After this, decollapse happens on first item and all accordion items are decollapsed
click "Collapsible Group Item Move pf-utilz.bar.chart to own repo., created generic pf doc page #1" again (nothing happens)

Expected behavior here would be for Collapsible Group Item #1 to collapse again. Can you reproduce it?

priley86 · 2017-04-09T22:06:04Z

also - please squash commits when you get a chance 😸

recrsn · 2017-04-10T01:24:13Z

upon first load click "Collapsible Group Item #1" (collapsed) in first accordion. After this, decollapse happens on first item and all accordion items are decollapsed

@priley86 Did the collapsible group #1 appear as collapsed ? It has the open attribute, therefore it should be expanded initially.

priley86 · 2017-04-10T02:01:51Z

@Agathver yes, it is expanded initially (which is fine). however, once clicked, it becomes decollapsed (expected), and once clicked again, it is frozen (which is unexpected). Can you try this?

actually it seems all accordion items have same behavior...they will only expand once...

priley86 · 2017-04-10T12:57:33Z

hey @Agathver , one other thing I'd like to test w/ this is handling content changes (most commonly a framework will render nested bindings inside of your CE's template). What's important to note about this is framework lifecycle exists outside of CE lifecycle, so one way simple way to handle this is to expose a handleContentChanged event in each CE which could have content changes via the framework. There is an example of this in the pf-tooltip. You can test this w/ a framework, but the easiest way is to just query the DOM and manipulate it via JS, and then trigger the event (also in the pf-tooltip test page). Can we do something like this here too?

recrsn · 2017-04-12T15:31:21Z

@priley86 Here, we do not modify the user supplied DOM in any way, so frameworks should have no issues. I'll test them with Angular (1&2) and React at a later date after I finish my crash course on React this weekend.

Adding panels dynamically also should work fine. Only the child of pf-accordion-heading with data-toggle="collapse" may not be removed dynamically.

I think a found a way to break it though ;)

For some reason, the ontransitionend function is not being called properly when the animation is started with attachedCallback. I'm working on a workaround.

priley86 · 2017-04-12T18:55:38Z

great! thanks so much @Agathver. It's also great to hear you are learning more about Angular & React! This will not only help w/ Web Component development, but will enable you to build more apps and ensure the web components work well everywhere (our main goal!). I think familiarity w/ these frameworks will help your career regardless though.

Also thanks for looking at this issue I noted...this PR is looking pretty close after that. Are you able to update the events after our new event naming convention?

Keep up the great work 💯 😺

recrsn · 2017-04-14T03:56:33Z

I am facing this strange issue. The collapse and expand work fine when a debugger is attached, but without the debgger, it doesn't.

recrsn · 2017-04-14T16:51:54Z

Hi @priley86 the panel collapsing issue should be fixed. Please check and let me know.

When everything is fine, I'll squash all the commits.

priley86 · 2017-04-17T13:04:24Z

awesome @Agathver, this looks great! I confirmed too that this bug is fixed in your test page. Great job!

do you mind updating the event names based on our new convention? (<pf-component-name>.eventName etc etc)

also, would love to see an Angular or React plunker on this just to be sure it works out...but it looks fine to me :)

Let me know if I can help any further.

priley86 · 2017-04-17T13:02:14Z

+      if (this._target._initialized) {
+        this._initializeToggle();
+      } else {
+        this._target.addEventListener('pf-accordion-initialized', () => {


pf-accordion.initialized

priley86 · 2017-04-17T13:02:35Z

+          this._toggle.classList.add('collapsed');
+          this._toggle.setAttribute('aria-expanded', 'false');
+        }
+        this._target.addEventListener('pf-accordion-expanding', () => {


pf-accordion.expanding

priley86 · 2017-04-17T13:02:46Z

+          this._toggle.classList.remove('collapsed');
+          this._toggle.setAttribute('aria-expanded', 'true');
+        });
+        this._target.addEventListener('pf-accordion-collapsing', () => {


pf-accordion.collapsing

recrsn · 2017-04-17T16:33:09Z

@priley86 I have updated the events to the new convention.

priley86 · 2017-04-18T13:05:55Z

@Agathver awesome! this looks amazing.

I think we are almost there. Let's just test this out now in Angular/React...Do you have a plnkr up we can use? Eventually it will be nice to add examples to our ng2/React repos, but I think we can safely merge this PR beforehand if we just confirm it works...

Thanks so much!!!

recrsn · 2017-04-29T19:18:49Z

pf-accordion Demos with other frameworks

with React: https://jsfiddle.net/bnsr1anc/
with Angular2:https://embed.plnkr.co/w0UxgN/

priley86 · 2017-05-01T13:18:42Z

@Agathver the demos look great! I think this adequately demonstrates the WC and its functionality cross framework. Small note about the React demo, we can add the panel-title class to the h4 titles to get the title icons. Don't worry about it now, we can do that on the live site demo later!

I'll await other reviewers to chime in and see if we can't improve anything else...

priley86 · 2017-05-01T13:26:51Z

any more thoughts @dlabrecq @bleathem @cdcabrera ?

recrsn · 2017-05-25T11:25:04Z

Since we have moved to CE v1 this PR needs to be updated too.

recrsn · 2017-06-04T17:46:14Z

Looks like travis won't let code with mutation observers to pass #45

priley86 · 2017-06-04T20:22:55Z

Thanks for reporting this @Agathver (and thanks very much for upgrading pf-accordion to v1)!

If we have to hold off from testing Mutation Observer w/ Travis/ Headless Chrome for the time being, I think that's OK. Headless Chrome definitely seems to be a better option than Phantom.

Any idea if there is an issue opened w/ the Travis team we can link to? This is cutting edge research so thanks so much for looking into it 👍

recrsn · 2017-06-05T07:36:18Z

@priley86 It really looks something else is happening, like callbacks being called in async manner or the connectedCallback not being called at all!

I even tried installing Chrome 58 in travis and it still fails. Here is my approach: https://github.com/Agathver/patternfly-webcomponents/blob/travis-test/.travis.yml

While the same version works in local
Local: Chromium 58.0.3029.110 Fedora Project PASS
Local (desktop/Ubuntu 16.04): Google Chrome 58.0.3029.110 PASS
Travis: Google Chrome 58.0.3029.110 FAIL 😞

dabeng · 2017-06-07T17:26:26Z

@Agathver , Headless Chrome is shipping in Chrome 59+.

recrsn · 2017-06-07T18:20:07Z

@dabeng Headless landed in Chrome/Chromium 58 for Linux, 59 for macOS abd others. However now that Chrome 59 is out, we still should be able to get it running.

The travis log clearly shows that, the script errors out while executing a test that needs MutationObservers.

dabeng · 2017-06-16T02:50:29Z

+  /**
+   * Called when an instance was inserted into the document
+   */
+  connctedCallback() {


It should be connectedCallback().

This typo! Thanks!

dabeng · 2017-06-16T02:55:40Z

+   * Called when the element is removed from the DOM
+   */
+  disconnectedCallback() {
+    this._observer.disconnect();


this._observer will be undefined on pf-accordion-heading being removed. So just remove this sentence.

I moved the initialization of MutationObserver to constructors, hence it should not be undefined. However, as I understood the v1 spec, the connectedCallback should be executed before disconnectedCallback

dabeng · 2017-06-16T02:56:52Z

+   * Called when the element is removed from the DOM
+   */
+  disconnectedCallback() {
+    this._observer.disconnect();


this._observer will be undefined on pf-accordion being removed. So just remove this sentence.

dabeng · 2017-06-16T03:03:06Z

Hi @Agathver , I just put forward three recommendations. I believe you can get Travis working again If you try them out.

dabeng · 2017-06-18T22:18:54Z

@priley86 , Hi patrick, maybe you have the appropriate right to modify the amitosh's PR. I have found some snippets which should be corrected.

priley86 · 2017-06-20T11:38:42Z

hey @dabeng - i think it is fine to branch @Agathver's branch and push a new one if you'd like. You can pull request into his branch or start a new PR that we can reference in this PR (no issue at all w/ ongoing PRs here if you want to help out). @Agathver, thanks a lot for helping us get this far w/ pf-accordion and @dabeng thanks for the reviews!

recrsn · 2017-06-20T11:43:57Z

@dabeng @priley86 I would definitely appreciate your help in this PR. I'm currently bit occupied with my GSoC work, and travis isn't being bit friendly to me these days.

I have added some commits, which narrowed the failing tests to a single one. However, the tests runs fine on newer Chrome.

* Closes patternfly-webcomponents#25

priley86 · 2017-08-15T14:58:36Z

I've just tested this component across browsers and noted issue #72 w/ @agathver.

Merging this so we can begin consuming this component now and will follow up on #72 in subsequent PRs.

recrsn force-pushed the pf-accordion branch from 914ab50 to fee6742 Compare March 22, 2017 16:23

recrsn force-pushed the pf-accordion branch from 0c346dc to 345068e Compare April 3, 2017 20:16

recrsn changed the title ~~WIP : pf-accordion component~~ pf-accordion component Apr 3, 2017

priley86 suggested changes Apr 6, 2017

View reviewed changes

recrsn force-pushed the pf-accordion branch from 8f38959 to 673f32f Compare April 14, 2017 04:21

priley86 reviewed Apr 17, 2017

View reviewed changes

recrsn force-pushed the pf-accordion branch from 5e1ffad to 18ccff6 Compare April 17, 2017 16:04

recrsn force-pushed the pf-accordion branch 2 times, most recently from fda7ee8 to 831bc76 Compare April 19, 2017 16:28

recrsn force-pushed the pf-accordion branch 2 times, most recently from ae6adbe to bc9a032 Compare April 29, 2017 19:11

priley86 approved these changes May 1, 2017

View reviewed changes

recrsn mentioned this pull request May 15, 2017

Add pf-accordion to showcase patternfly-webcomponents/patternfly-webcomponents.github.io#1

Open

3 tasks

recrsn force-pushed the pf-accordion branch from 2091f51 to 89b6805 Compare May 15, 2017 17:02

recrsn force-pushed the pf-accordion branch from 89b6805 to 6310733 Compare June 5, 2017 06:26

dabeng reviewed Jun 16, 2017

View reviewed changes

recrsn force-pushed the pf-accordion branch from 6310733 to 435e4fa Compare June 16, 2017 03:32

recrsn force-pushed the pf-accordion branch from 435e4fa to 827728a Compare July 5, 2017 18:27

priley86 added enhancement help wanted labels Aug 14, 2017

recrsn force-pushed the pf-accordion branch 5 times, most recently from 131bd99 to f26173a Compare August 15, 2017 13:53

feat(accordion): adds new pf-accordion component

3510ee8

* Closes patternfly-webcomponents#25

recrsn force-pushed the pf-accordion branch from f26173a to 3510ee8 Compare August 15, 2017 14:35

priley86 merged commit a8986dc into patternfly-webcomponents:master Aug 15, 2017

recrsn deleted the pf-accordion branch August 15, 2017 15:26

Uh oh!

Conversation

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

Uh oh!

Uh oh!

priley86 commented Mar 15, 2017

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

priley86 left a comment

Choose a reason for hiding this comment

Uh oh!

priley86 Apr 6, 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 Apr 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

recrsn commented Apr 10, 2017

Uh oh!

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

Uh oh!

Uh oh!

priley86 commented Apr 10, 2017

Uh oh!

recrsn commented Apr 12, 2017

Uh oh!

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

Uh oh!

Uh oh!

recrsn commented Apr 14, 2017

Uh oh!

recrsn commented Apr 14, 2017

Uh oh!

priley86 commented Apr 17, 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!

recrsn commented Apr 17, 2017

Uh oh!

priley86 commented Apr 18, 2017

Uh oh!

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

Uh oh!

Uh oh!

priley86 commented May 1, 2017

Uh oh!

priley86 commented May 1, 2017

Uh oh!

recrsn commented May 25, 2017

Uh oh!

recrsn commented Jun 4, 2017

Uh oh!

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

Uh oh!

Uh oh!

recrsn commented Jun 5, 2017

Uh oh!

dabeng commented Jun 7, 2017

Uh oh!

recrsn commented Jun 7, 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

recrsn commented Mar 13, 2017 •

edited

Loading

recrsn commented Mar 15, 2017 •

edited

Loading

recrsn commented Apr 3, 2017 •

edited

Loading

priley86 Apr 6, 2017 •

edited

Loading

priley86 commented Apr 9, 2017 •

edited

Loading

priley86 commented Apr 9, 2017 •

edited

Loading

priley86 commented Apr 10, 2017 •

edited

Loading

priley86 commented Apr 12, 2017 •

edited

Loading

priley86 commented Apr 17, 2017 •

edited

Loading

recrsn commented Apr 29, 2017 •

edited

Loading

priley86 commented Jun 4, 2017 •

edited

Loading

priley86 commented Jun 20, 2017 •

edited

Loading

priley86 commented Aug 15, 2017 •

edited

Loading