Skip to content

Conversation

@4very
Copy link
Contributor

@4very 4very commented Nov 21, 2019

Related Issues
#93

Describe the Changes
Implemented lightbox2 javascipt to allow arrow key functionality for galleries as well as better background blurring and user prompting. This is applied to featured photos, galleries, and any inline photos.

Still To Do
Nothing.

Problems
If the javascript doesnt properly execute (which i havent had happen to me) clicking on the images will redirect to the images direct link.

Testing
checked articles that contain every combination of featured photo, galleries, and inline photos. As well as articles that contain none.

@4very 4very requested review from jlyon1 and pantp November 21, 2019 03:30
@4very
Copy link
Contributor Author

4very commented Nov 21, 2019

getting this error while running tests, cannot figure out why. Any help would be appreciated.

ValueError: Missing staticfiles manifest entry for '/js/lightbox.js'

jlyon1
jlyon1 previously requested changes Nov 21, 2019
Copy link
Member

@jlyon1 jlyon1 left a comment

Choose a reason for hiding this comment

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

We shouldn't include the raw lightbox.js file in the repo, instead it should be included with webpack similar to how stimulus.js is included

@4very
Copy link
Contributor Author

4very commented Nov 21, 2019

We shouldn't include the raw lightbox.js file in the repo, instead it should be included with webpack similar to how stimulus.js is included

I have made changes to the js file in order for it to work with pipeline.

@jlyon1
Copy link
Member

jlyon1 commented Nov 21, 2019

We should treat libraries as a black box, that way we can keep them up to date, manage them with npm/webpack, and swap them out with equivalent or similar libraries if need be. Additionally if we merge in a large js blob like this we will have a much more difficult time pinning down any bugs that may arise, especially if they don't appear in our source code.

If you need functionality the library doesn't provide, you can add the code into the pipeline source to make the modification.

@4very 4very requested a review from jlyon1 December 18, 2019 22:13
@4very
Copy link
Contributor Author

4very commented Dec 18, 2019

Added the changes requested, the command that is failing in the build is working on my machine and could not find a solution for.

@4very 4very dismissed jlyon1’s stale review December 27, 2019 00:40

changes have been made

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.

4 participants