fix: Update accordion to use PfeCollapse #1642

castastrophe · 2021-05-25T12:19:33Z

Testing instructions

Component design matches mock-ups pfe-accordion #877
Click toggles the accordion state
Space or enter toggles the accordion state

Browser requirements

Your component should work in all of the following environments:

Latest 2 versions of Edge
Internet Explorer 11 (should be useable, not pixel perfect)
Latest 2 versions of Firefox (one on Mac OS, one of Windows OS)
Firefox 78 (or latest version for Red Hat Enterprise Linux distribution)
Latest 2 versions of Chrome (one on Mac OS, one of Windows OS)
Latest 2 versions of Safari
Android mobile device (such as the Galaxy S9)
Apple mobile device (such as the iPhone X)
Apple tablet device (such as the iPhone Pro)

Ready-for-merge Checklist

Expected files: all files in this pull request are related to one request or issue (no stragglers or scope-creep).
Tests have been updated to cover these changes.
Browser testing passed.
Changelog updated.
Documentation (README.md, WHY.md, etc.) updated or added.
Link to the demo recording:
Approved by designer.

Merging

Please squash when merging and ensure your commit message uses conventional commit formatting.

Be sure to share your updates with the [email protected] mailing list!

netlify · 2021-07-15T15:11:03Z

✔️ Deploy Preview for patternfly-elements ready!

🔨 Explore the source changes: fb55dd4

🔍 Inspect the deploy log: https://app.netlify.com/sites/patternfly-elements/deploys/60fb23879fee120008a792eb

😎 Browse the preview: https://deploy-preview-1642--patternfly-elements.netlify.app

castastrophe · 2021-07-15T20:28:43Z

elements/pfe-jump-links/src/pfe-jump-links-panel.js

    this.sectionMargin = newVal;
  }

-  _isValidMarkup() {


I was seeing this error on the pfe-accordion demo page but really it was just missed in the previous PR for jump links. This rule is no longer in place (content can exist at any level inside the DOM).

Can we move this into a separate issue?

…to fix-accordion-alignment-kit

docs/main.css

…atternfly-elements into fix-accordion-alignment-kit

kylebuch8 · 2021-07-20T22:47:44Z

@castastrophe - This is a pretty big change and its a ton to take in at once. Can we make some smaller, more incremental changes? The design system changes would be the first priority. Then we can look at extending pfe-accordion with pfe-collapse.

castastrophe · 2021-07-20T23:10:10Z

I hear you but at this point I'm focusing on getting code I have handed off or merged. I'm short on time and need this in order to move forward with nav 1.0 patches.

kylebuch8 · 2021-07-21T12:12:42Z

Ok but we're going to need some work on the animations. The closing animation is no longer working and the opening animation has some jank. I tried to see why the opening animation is no longer smooth but nothing jumped out to me.

castastrophe · 2021-07-21T13:20:35Z

Ok but we're going to need some work on the animations. The closing animation is no longer working and the opening animation has some jank. I tried to see why the opening animation is no longer smooth but nothing jumped out to me.

This is a recording of Chrome, Firefox, and then Safari:

Screen.Recording.2021-07-21.at.9.17.37.AM.mov

It's not a perfect animation but it's on par with what we have now.

coreyvickery · 2021-07-26T22:08:45Z

@castastrophe @kylebuch8 A few comments.

Accordion

Themes

Typography

Are we able to get the panel title/headline closer to what's in the kit?
Type is very close, font should be 20pt/30 line height and Medium weight (I believe that's 400)
Correct specs below

Arrow

Kit

Demo

Are we able to get the arrow size and thickness closer to what's in the kit?
Correct specs below

Hover state

There appears to be vertical lines on the left and right on hover (correct example below), please delete
Maintain the blue accent line and gray background color on hover
Correct styles below

Mobile

One-column only layouts on phone breakpoints, no two-column layouts

Disclosure

Disclosure should have a border around the entire panel, not top and bottom only (looks correct on hover only)
Text should be left aligned near the arrow
Are we able to get the arrow size and thickness closer to what's in the kit?
Same arrow as Accordion, just aligned on left with 8px of spacing in between the text
Correct specs below

castastrophe · 2021-07-27T17:46:21Z

@coreyvickery I've isolated the design updates into a separate PR per @kylebuch8's request and implemented your feedback here: #1726

…to fix-accordion-alignment-kit

castastrophe · 2021-07-29T22:10:03Z

elements/pfe-accordion/src/pfe-accordion-header.js

-  get templateUrl() {
-    return "pfe-accordion-header.html";
+  get html() {
+    // return `<slot></slot>`;


Suggested change

// return `<slot></slot>`;

castastrophe added 3 commits May 24, 2021 10:12

fix: Update pfe-accordion to align with the kit; remove extends

4e358a8

fix: Aligning with the design kit styles

02bddeb

fix: Update pfe-accordion with design kit mock-ups

5cc66f6

castastrophe marked this pull request as draft May 25, 2021 12:19

This was linked to issues May 25, 2021

[fix] pfe-accordion style issues with chevron and interaction states #1512

Closed

[feat] pfe-accordion | Cannot add custom code in accordion headers #1403

Closed

castastrophe added 4 commits May 25, 2021 11:44

fix: Align styles for accordion to the kit; resolve remaining hangnails

252fc37

fix: Working through migration to pfeCollapse

af77de2

fix: Working through necessary API for collapse

4e45143

fix: Updating the animation timing

e14999e

castastrophe added 2 commits July 15, 2021 12:06

fix: Resolving looping on expanded state

2bde480

fix: Working through expansion duplication

fdd87ff

castastrophe commented Jul 15, 2021

View reviewed changes

castastrophe added 4 commits July 16, 2021 11:21

fix: Working through typography updates

d6b1501

fix: Update documentation; add back active hover state

1cd059d

fix: Replace HP references with lorem ipsum

c1a604c

Merge branch 'master' of github.com:patternfly/patternfly-elements in…

d1dcf88

…to fix-accordion-alignment-kit

github-actions bot added demo Updating demo pages docs Documentation updates work in progress POC / Not ready for review functionality Functionality, typically pertaining to the JavaScript. styles An issue or PR pertaining only to CSS/Sass tests Related to testing labels Jul 16, 2021

castastrophe commented Jul 16, 2021

View reviewed changes

docs/main.css Outdated Show resolved Hide resolved

castastrophe added 4 commits July 16, 2021 12:14

Remove commented out styles

593251f

fix: Update pfe-accordion demo page

aec3dfe

Merge branch 'fix-accordion-alignment-kit' of github.com:patternfly/p…

1a3a641

…atternfly-elements into fix-accordion-alignment-kit

fix: Opt-out of the keypress event when using semantic buttons

dea3e2e

castastrophe removed the work in progress POC / Not ready for review label Jul 16, 2021

castastrophe added 2 commits July 20, 2021 15:06

fix: Repair keyboard functionality

6f9ac85

fix: Fix failing test

30a703d

castastrophe and others added 5 commits July 21, 2021 10:19

fix: Working through updating test cases and fixing Safari bugs

106e709

fix: Fix accordion chevron color in safari

467b506

fix: Collate polyfills

5df1573

fix: Patch band issue in IE11 found when debugging accordion

612f4bb

fix: working through IE11 issues and adding comments

fb55dd4

castastrophe marked this pull request as draft July 26, 2021 21:00

castastrophe changed the title ~~fix: Accordion component design alignment with the kit~~ fix: Update accordion to use PfeCollapse Jul 27, 2021

Merge branch 'master' of github.com:patternfly/patternfly-elements in…

42e14ec

…to fix-accordion-alignment-kit

github-actions bot added work in progress POC / Not ready for review AT passed Automated testing has passed and removed demo Updating demo pages labels Jul 29, 2021

castastrophe commented Jul 29, 2021

View reviewed changes

castastrophe and others added 7 commits July 29, 2021 18:15

fix: revert header space changes

9183642

Branch was auto-updated with the latest.

ab7d197

Branch was auto-updated with the latest.

17bfebc

Branch was auto-updated with the latest.

d5fe5e6

Branch was auto-updated with the latest.

1c47816

Branch was auto-updated with the latest.

8a7ab92

Branch was auto-updated with the latest.

f7830eb

kylebuch8 removed a link to an issue Sep 10, 2021

[fix] pfe-accordion style issues with chevron and interaction states #1512

Closed

castastrophe closed this Oct 13, 2021

bennypowers deleted the fix-accordion-alignment-kit branch June 25, 2024 07:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Update accordion to use PfeCollapse #1642

fix: Update accordion to use PfeCollapse #1642

Uh oh!

castastrophe commented May 25, 2021 •

edited

Loading

Uh oh!

netlify bot commented Jul 15, 2021 •

edited

Loading

Uh oh!

castastrophe Jul 15, 2021

Uh oh!

kylebuch8 Jul 20, 2021

Uh oh!

Uh oh!

kylebuch8 commented Jul 20, 2021

Uh oh!

castastrophe commented Jul 20, 2021

Uh oh!

kylebuch8 commented Jul 21, 2021

Uh oh!

castastrophe commented Jul 21, 2021

Uh oh!

coreyvickery commented Jul 26, 2021

Uh oh!

castastrophe commented Jul 27, 2021

Uh oh!

castastrophe Jul 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix: Update accordion to use PfeCollapse #1642

fix: Update accordion to use PfeCollapse #1642

Uh oh!

Conversation

castastrophe commented May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing instructions

Browser requirements

Ready-for-merge Checklist

Merging

Uh oh!

netlify bot commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

castastrophe Jul 15, 2021

Choose a reason for hiding this comment

Uh oh!

kylebuch8 Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kylebuch8 commented Jul 20, 2021

Uh oh!

castastrophe commented Jul 20, 2021

Uh oh!

kylebuch8 commented Jul 21, 2021

Uh oh!

castastrophe commented Jul 21, 2021

Uh oh!

coreyvickery commented Jul 26, 2021

Accordion

Themes

Typography

Arrow

Kit

Demo

Hover state

Mobile

Disclosure

Uh oh!

castastrophe commented Jul 27, 2021

Uh oh!

castastrophe Jul 29, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

castastrophe commented May 25, 2021 •

edited

Loading

netlify bot commented Jul 15, 2021 •

edited

Loading