Skip to content

fix: Get nested accordions and tabs working inside the content-set #975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
kylebuch8 merged 10 commits into from
Jul 15, 2020
Merged

fix: Get nested accordions and tabs working inside the content-set #975

kylebuch8 merged 10 commits into from
Jul 15, 2020

Conversation

@castastrophe
Copy link
Contributor

@castastrophe castastrophe commented Jul 13, 2020 •
edited
Loading

If an accordion or tab is nested inside a pfe-content-set component, the component should upgrade with no errors.

Related issues

Preview

Link(s) to demo page(s) where this element can be viewed:

What has changed and why

Summarize files edited as part of this MR along with a brief description of what was changed/why.

  • Added example nested tab and accordion on the pfe-content-set demo page
  • Assign a random value to pfe-id attribute on the upgraded tab or accordion wrapper
  • Remove on from the schema
  • Add test cases
  • Prefix all random IDs with pfe- so that they never fail on starting with a number

Testing instructions

Be sure to include detailed instructions on how your update can be tested by another developer.

  1. On the demo page, navigate down to the content-set under the heading: "Accordion nested inside a content-set component"
  2. Adjust your window to 600px and refresh.
  3. The component should upgrade to accordions.
  4. Open the first accordion item. You should see a nested accordion.
  5. Open the second accordion item. You should see a nested accordion.
  6. Increase your browser width to 1200px. Refresh.
  7. The component should upgrade to tabs.
  8. Open the third tab item. You should see a nested tabset.

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 68 (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

Check off items as they are completed. Feel free to delete items if they are not applicable.

  • 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.
  • Repository compiles and tests pass.
  • Changelog updated.
  • Link to the demo recording:

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!

@castastrophe castastrophe added demo Updating demo pages functionality Functionality, typically pertaining to the JavaScript. tests Related to testing labels Jul 13, 2020
@castastrophe castastrophe linked an issue Jul 13, 2020 that may be closed by this pull request
pfe-content-set: Bug in mobile view if pfe-content-set--panel contains an accordion #968
Closed
@castastrophe castastrophe self-assigned this Jul 13, 2020
@castastrophe castastrophe requested a review from kylebuch8 July 13, 2020 17:54
@castastrophe castastrophe added bug ready: branch testing Test the component from a user-perspective. Try to break it! ready: browser testing Test the component in the supported browser environments. ready: code review Ready for code review! priority: high Severity level: 1 labels Jul 13, 2020
@castastrophe castastrophe removed ready: branch testing Test the component from a user-perspective. Try to break it! ready: browser testing Test the component in the supported browser environments. labels Jul 15, 2020
kylebuch8
kylebuch8 approved these changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@kylebuch8 kylebuch8 left a comment

Choose a reason for hiding this comment

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

Launching Garbage To Moon 🌚

@kylebuch8 kylebuch8 merged commit 6965df7 into master Jul 15, 2020
@kylebuch8 kylebuch8 deleted the fix-nested-accordion-content-set branch July 15, 2020 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kylebuch8 kylebuch8 kylebuch8 approved these changes

Assignees

@castastrophe castastrophe

Labels

demo Updating demo pages functionality Functionality, typically pertaining to the JavaScript. priority: high Severity level: 1 ready: code review Ready for code review! tests Related to testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

pfe-content-set: Bug in mobile view if pfe-content-set--panel contains an accordion

3 participants

@castastrophe @kylebuch8