Skip to content
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.

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

Navlink | Bug | Prevent JS errors in Navlink When Clicking on "Non Scroll-to" Links #1322

Merged
merged 4 commits into from Aug 9, 2019

Conversation

sghoweri
Copy link
Contributor

@sghoweri sghoweri commented Aug 7, 2019

Jira

http://vjira2:8080/browse/BDS-1728

Summary

Updates the Navlink JS to fix JS errors getting thrown if a link that gets clicked on isn't a link that would get scrolled to.

Note: This error can be viewed on the live boltdesignsystem.com by opening the browser console on the homepage and clicking on the Github link that opens in a new tab.

Details

  • Adds missing super calls to the Navlink's older JS
  • Add missing config to disable Shadow DOM in the Navlink component (just a precaution given that this component isn't currently rendering / re-rendering any HTML)
  • Update the Navlink JS logic to account for links that don't have an href, aren't a hashed link to scroll to an element on the page, or don't have a matching element to scroll to

How to test

      {
        text: "No Link",
        url: ""
      },
      {
        text: "Non-existent Element",
        url: "#foo"
      },
  • Locally, on the main docs site homepage, click on a few different Navlinks in the Navbar to test out the following use cases:

    1. Clicking on a navbar nav link that doesn’t contain an href doesn’t throw a JS error
    2. Clicking on a navbar nav link with a hashed URL but without a matching HTML element with the expected ID doesn’t throw a JS error
    3. Clicking on a navbar nav link with a regular page URL (non-hash URL) doesn’t throw a JS error like it does currently. NOTE: this is easier to replicate when clicking on a URL that opens in a new tab!
  • Navigate to the sticky navbar demo page and confirm that when clicking on a navbar nav link that DOES have a matching HTML element with the expected ID, the matching element is smooth scrolled into view as expected.

…t (temp workaround to component not currently rendering HTML)
…e an href, aren't a hashed link to scroll to an element on the page, or don't have a matching element to scroll to
Copy link
Collaborator

@remydenton remydenton left a comment

Choose a reason for hiding this comment

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

Looks good, tested and works as expected.

For my own benefit, is there a back story/reference with more details about calling super() in the connector/disconnector methods?

@sghoweri
Copy link
Contributor Author

sghoweri commented Aug 9, 2019

Looks good, tested and works as expected.

For my own benefit, is there a back story/reference with more details about calling super() in the connector/disconnector methods?

@remydenton add that to the list of stuff to chat about during our next Dev huddle!

TLDR: super is super important to call when we're dealing with multiple layers of Classes that open times have have inherited behavior and defaults!

Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

@sghoweri Tested locally, works as expected. Approved 👍

@sghoweri sghoweri merged commit 31c79db into master Aug 9, 2019
@sghoweri sghoweri deleted the fix/navlink-js-fix branch August 9, 2019 15:38
@sghoweri sghoweri mentioned this pull request Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants