Skip to content

feat(accordion): 1:1#2114

Merged
bennypowers merged 47 commits intomainfrom
feat/pfe-accordion-1-to-1
Oct 12, 2022
Merged

feat(accordion): 1:1#2114
bennypowers merged 47 commits intomainfrom
feat/pfe-accordion-1-to-1

Conversation

@brianferry
Copy link
Copy Markdown
Collaborator

@brianferry brianferry commented Aug 25, 2022

What I Did

  • Removed all SCSS specific styles and replaced with CSS friendly styles
  • Updated the stylings for pfe-accordion to match pf-accordion
  • Created base classes for all of the pfe-accordion classes that will share the logic between PFE and RHDS accordions. Classes include:
    • BaseAccordion
    • BaseAccordionHeader
    • BaseAccordionPanel
  • Implemented the following attributes for pfe-accordion
    • single
      • Only a single panel for an accordion can be open at a time
    • fixed
      • pfe-accordion-panel specific
      • If text goes over a maximum height (default 9.375rem) a scrollbar is added to the panel vertically
    • bordered
      • Adds a border between the headers in the accordion & on the top / bottom of the accordion.
    • large
      • Makes the font / spacing larger for the component.
      • Automatically gets the bordered styles
  • Fixed an issue where nested accordions did not behave correctly to the single attribute by looking for the nearest accordion parent element and performing the expand / collapse check on it instead of bubbling all the way to the top level component.

Choices

  • Disclosure has been removed from pfe-accordion and will be implemented back into the RHDS accordions rh-accordion

To Do

  • Implement spec test files for the new attributes
  • Update the css props with the updated attributes

Testing Instructions

Note To Reviews

This PR is breaking and will align pfe-accordion with the PFv4 accordion element.

Closes #2047

Example patternfly.org accordion component

<Accordion>
  <AccordionItem>
    <AccordionToggle onClick={() => { onToggle('def-list-toggle1'); }} isExpanded={expanded === 'def-list-toggle1'} id="def-list-toggle1">
        Item one
      </AccordionToggle>
      <AccordionContent id="def-list-expand1" isHidden={expanded !== 'def-list-toggle1'}>
        <p>
          Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et
          dolore magna aliqua.
        </p>
       </AccordionContent>
      </AccordionItem>
  </Accordion>

Example proposed patternfly-elements accordion component

<pfe-accordion>
  <pfe-accordion-header>
    <h4>Accordion header 1</h4>
  </pfe-accordion-header>
  <pfe-accordion-panel>
      <p>Lorem, ipsum dolor sit amet consectetur adipisicing elit. Quos eius architecto sint sed provident. Fugiat quibusdam voluptas eveniet, facere repellendus aperiam aliquid voluptate est porro magni itaque laboriosam voluptatem modi?</p>
    </pfe-accordion-panel>
</pfe-accordion>

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Aug 25, 2022

🦋 Changeset detected

Latest commit: 567bc14

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@patternfly/pfe-accordion Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions Bot added work in progress POC / Not ready for review demo Updating demo pages docs Documentation updates functionality Functionality, typically pertaining to the JavaScript. styles An issue or PR pertaining only to CSS/Sass labels Aug 25, 2022
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 25, 2022

Deploy Preview for patternfly-elements ready!

Name Link
🔨 Latest commit ada08fe
😎 Deploy Preview https://deploy-preview-2114--patternfly-elements.netlify.app/

_To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions Bot added the AT passed Automated testing has passed label Aug 25, 2022
@bennypowers
Copy link
Copy Markdown
Member

make sure to add a changeset and keep it up to date with api changes

@github-actions github-actions Bot added the tests Related to testing label Aug 29, 2022
@bennypowers
Copy link
Copy Markdown
Member

some notes from system fed discovery:

  • we'd prefer to remove all the url state code from this component, so that it becomes an app-level concern instead
  • similarly, expanded-index attr should be removed in favour of expanded attr on individual panels. the js property can stay though, and so could any sort of expandIndex(i: number): void method

Comment thread elements/pfe-accordion/BaseAccordionHeader.scss Outdated
Comment thread elements/pfe-accordion/BaseAccordionHeader.scss Outdated
Copy link
Copy Markdown
Contributor

@marionnegp marionnegp left a comment

Choose a reason for hiding this comment

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

Added one comment. The nested accordions look good!

Comment thread elements/pfe-accordion/BaseAccordionHeader.scss Outdated
@brianferry brianferry added ready: code review Ready for code review! and removed work in progress POC / Not ready for review labels Oct 5, 2022
Comment thread elements/pfe-accordion/BaseAccordionHeader.scss Outdated
Comment thread elements/pfe-accordion/BaseAccordionHeader.scss Outdated
@bennypowers bennypowers merged commit 70795e8 into main Oct 12, 2022
@bennypowers bennypowers deleted the feat/pfe-accordion-1-to-1 branch October 12, 2022 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AT passed Automated testing has passed demo Updating demo pages docs Documentation updates functionality Functionality, typically pertaining to the JavaScript. ready: code review Ready for code review! styles An issue or PR pertaining only to CSS/Sass tests Related to testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[1:1] pfe-accordion

3 participants