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

Fixed Magellan issue when target does not exist. #9070

Merged
merged 1 commit into from
Sep 27, 2016

Conversation

Owlbertz
Copy link
Contributor

Prevent any scrolling action when the target of the clicked link does not exist.
Previously the .getOffset().top created an JS error because Cannot read property 'top' of undefined.
Also added a basic test for scrolling and a test for the graceful failing when the target does not exist.

Noticed this issue when checking #9066.

Prevent any scrolling action when the target of the clicked link does not exist.
Previously the `.getOffset().top` created an JS error because `Cannot read property 'top' of undefined`.
Also added a basic test for scrolling and a test for the graceful failing when the target does not exist.
@Owlbertz
Copy link
Contributor Author

Any feedback from @zurb/yetinauts ?

@coreysyms
Copy link
Contributor

This looks good, would we want a console log stating there is "no target, please check existence or spelling". Just a thought. Otherwise, merge it!

@Owlbertz
Copy link
Contributor Author

@coreysyms Not sure if there is any Foundation-wide guideline on console logs. Maybe @kball can help?

@kball
Copy link
Contributor

kball commented Sep 27, 2016

This looks good; I've fought with exactly this bug before.

On console.logs... historically we don't include them in the codebase. I think @coreysyms has a good idea... I wonder if we should have a set of debug only logs that get compiled out in the production builds... for now, lets not worry about it. Going to merge both this and #9066.

@kball kball merged commit 3e9204b into foundation:develop Sep 27, 2016
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.

3 participants