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

fix: fix arrow navigation #121

Merged
merged 13 commits into from
Dec 2, 2018
Merged

fix: fix arrow navigation #121

merged 13 commits into from
Dec 2, 2018

Conversation

frederickfogerty
Copy link
Contributor

@frederickfogerty frederickfogerty commented Nov 17, 2018

Description

This PR fixes a regression in a previous release of this library, where arrow navigation was broken.

The problem was that when the Closure compiler was integrated into this library, the way the settings were passed around the objects was broken as Closure rewrites object key names. This fixes that.

kapture 2018-11-17 at 16 01 52

Fixes #74

Bug Fix

  • All existing unit tests are still passing (if applicable)
  • [N/A] Add new passing unit tests to cover the code introduced by your PR
  • [N/A] Update the readme
  • [N/A] Update or add any necessary API documentation
  • Add some steps so we can test your bug fix
  • The PR title is in the conventional commit format: e.g. fix(<area>): fixed bug #issue-number
  • Add your info to the contributors array in package.json!

Steps to Test

Check that tests still pass.

To test locally:

  1. Clone PR and npm install && npm run build
  2. Run a webserver and open index.html in a browser
  3. Click on an image at the bottom
  4. Check that arrow navigation works

Copy link
Contributor

@jayeb jayeb left a comment

Choose a reason for hiding this comment

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

I assume the tests didn't catch this because they're not using the compiled library. Is that correct?

@frederickfogerty
Copy link
Contributor Author

@jayeb I don't believe there are any tests for this behavior - shall I add some?

@jayeb
Copy link
Contributor

jayeb commented Nov 20, 2018

Yes please!

…into fred/fix-arrow-nav

* 'fred/fix-arrow-nav' of https://github.com/imgix/luminous:
  chore(deps): update dependency karma-jasmine to v2.0.1 (#120)
  chore(deps): update dependency karma-jasmine to v2 (#119)
  chore(deps): update babel monorepo to v7.1.6 (#118)
  chore(deps): update dependency eslint-config-prettier to v3.3.0 (#117)
  chore(deps): update dependency eslint-config-prettier to v3.2.0 (#116)
  chore(deps): update dependency prettier to v1.15.2 (#115)
  chore(deps): update dependency eslint to v5.9.0 (#114)
@frederickfogerty
Copy link
Contributor Author

@jayeb Can you give this another review? I've added some tests for LuminousGallery

Copy link
Contributor

@jayeb jayeb left a comment

Choose a reason for hiding this comment

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

Tests look good to me, although Travis disagrees. Seems like the "navigate left" test is failing, but I can't immediately figure out why.

@frederickfogerty
Copy link
Contributor Author

There's something strange going on with Closure - I'll have to have a look tomorrow.

@frederickfogerty frederickfogerty merged commit 2be022a into master Dec 2, 2018
@frederickfogerty frederickfogerty deleted the fred/fix-arrow-nav branch December 2, 2018 15:49
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.

Can't get prev/next arrows to work
2 participants