FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: Update accordion to use PfeCollapse by castastrophe · Pull Request #1642 · patternfly/patternfly-elements · GitHub

/ patternfly-elements Public

fix: Update accordion to use PfeCollapse - #1642

Closed
castastrophe wants to merge 46 commits into
masterfrom
fix-accordion-alignment-kit
ConversationCommits (46)ChecksFiles changed
Closed

fix: Update accordion to use PfeCollapse#1642
castastrophe wants to merge 46 commits into
masterfrom
fix-accordion-alignment-kit

Conversation

castastrophe commented May 25, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 patternfly-elements-contribute@redhat.com mailing list!

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

netlify Bot commented Jul 15, 2021 •
edited
Loading

Copy link
Copy Markdown

✔️ 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 commented Jul 15, 2021
View reviewed changes
Comment thread elements/pfe-jump-links/src/pfe-jump-links-panel.js
this.sectionMargin = newVal;
}

_isValidMarkup() {

castastrophe Jul 15, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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).

kylebuch8 Jul 20, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can we move this into a separate issue?

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
Comment thread docs/main.css Outdated
castastrophe removed the work in progress POC / Not ready for review label Jul 16, 2021

kylebuch8 commented Jul 20, 2021

Copy link
Copy Markdown
Contributor

@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 commented Jul 20, 2021

Copy link
Copy Markdown
Contributor Author

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 commented Jul 21, 2021

Copy link
Copy Markdown
Contributor

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 commented Jul 21, 2021

Copy link
Copy Markdown
Contributor Author

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.

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

coreyvickery commented Jul 26, 2021

Copy link
Copy Markdown
Collaborator

@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 commented Jul 27, 2021

Copy link
Copy Markdown
Contributor Author

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

castastrophe changed the title fix: Accordion component design alignment with the kit fix: Update accordion to use PfeCollapse Jul 27, 2021
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
Comment thread elements/pfe-accordion/src/pfe-accordion-header.js
get templateUrl() {
return "pfe-accordion-header.html";
get html() {
// return `<slot></slot>`;

castastrophe Jul 29, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
// return `<slot></slot>`;

kylebuch8 removed a link to an issue Sep 10, 2021
[fix] pfe-accordion style issues with chevron and interaction states #1512
Closed
bennypowers deleted the fix-accordion-alignment-kit branch June 25, 2024 07:38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

kylebuch8 kylebuch8 left review comments

Assignees

No one assigned

Labels

AT passed Automated testing has passed docs Documentation updates functionality Functionality, typically pertaining to the JavaScript. styles An issue or PR pertaining only to CSS/Sass tests Related to testing tools Development and build tools work in progress POC / Not ready for review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[feat] pfe-accordion | Cannot add custom code in accordion headers pfe-disclosure pfe-accordion should extend pfe-collapse

3 participants

Back | FazBrowse Home | New Git URL