Skip to content

fix: Patch Jump links error #1224

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
castastrophe merged 24 commits into from
Dec 22, 2020
Merged

fix: Patch Jump links error #1224

castastrophe merged 24 commits into from
Dec 22, 2020

Conversation

@castastrophe
Copy link
Contributor

@castastrophe castastrophe commented Dec 11, 2020 •
edited
Loading

pfe-jump-links

A few missing fallback attributes in the properties for Jump links nav and panel were causing regressions in production.

Preview

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

What has changed and why

  • pfe-band: Remove flex styles on the .pfe-band__aside region because it wasn't in a flex container; substitute height: 100% to get the aside to fill its parent.
  • pfe-jump-links-nav: Add alias'ed attributes for autobuild and horizontal.
  • pfe-jump-links-panel: Add alias'ed attributes for offset and scrolltarget; convert this.menu_links to a node list rather than an array and wrap the matches actions in an if statement.

Testing instructions

  1. Test jump links inside the aside region of a band (sticky functionality should work). Link
  2. Test jump links with fallback attributes used. Link
  3. Check for regressions in the pfe-band aside region now that it is filling 100% height. Link

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 (not needed for documentation updates).
  • Documentation (README.md, WHY.md, etc.) updated or added.

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!

@github-actions github-actions bot added the functionality Functionality, typically pertaining to the JavaScript. label Dec 11, 2020
@castastrophe castastrophe added needs AT updates Please make updates to the automated testing. 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! needs docs updates labels Dec 11, 2020
@castastrophe castastrophe added this to the 1.x milestone Dec 11, 2020
@castastrophe castastrophe self-assigned this Dec 11, 2020
Djfaucette
Djfaucette approved these changes Dec 11, 2020
View reviewed changes
Copy link
Contributor

@Djfaucette Djfaucette left a comment

Choose a reason for hiding this comment

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

Lecture, gabble, timely messages

@github-actions github-actions bot added the styles An issue or PR pertaining only to CSS/Sass label Dec 11, 2020
@castastrophe castastrophe removed the ready: code review Ready for code review! label Dec 14, 2020
@castastrophe castastrophe linked an issue Dec 15, 2020 that may be closed by this pull request
pfe-jump-links inside of pfe-band aren't sticky #1063
Closed
@castastrophe castastrophe removed this from the 1.x milestone Dec 15, 2020
@castastrophe castastrophe linked an issue Dec 15, 2020 that may be closed by this pull request
pfe-jump-links | missing alias fallbacks for production markup #1233
Closed
@castastrophe castastrophe removed needs AT updates Please make updates to the automated testing. needs docs updates labels Dec 18, 2020
heyMP
heyMP approved these changes Dec 21, 2020
View reviewed changes
Copy link
Contributor

@heyMP heyMP left a comment

Choose a reason for hiding this comment

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

Changes look good!

@github-actions github-actions bot added ready to merge demo Updating demo pages docs Documentation updates tests Related to testing labels Dec 21, 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 Dec 21, 2020
@castastrophe castastrophe enabled auto-merge (squash) December 21, 2020 21:43
@castastrophe castastrophe merged commit 9671513 into master Dec 22, 2020
@castastrophe castastrophe deleted the fix-jump-links-js-error branch December 22, 2020 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Djfaucette Djfaucette Djfaucette approved these changes

@heyMP heyMP heyMP approved these changes

Assignees

@castastrophe castastrophe

Labels

demo Updating demo pages docs Documentation updates functionality Functionality, typically pertaining to the JavaScript. ready to merge styles An issue or PR pertaining only to CSS/Sass tests Related to testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

pfe-jump-links | missing alias fallbacks for production markup pfe-jump-links inside of pfe-band aren't sticky

4 participants

@castastrophe @heyMP @Djfaucette