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

Adds the ability to add offset to background position #10

Merged
merged 8 commits into from
Aug 18, 2017

Conversation

albertski
Copy link
Collaborator

Ran into some issues with the background image. I would see a black area above the image.

I used the same formula found here and it seems to work better:

var yPos = -($window.scrollTop() / $bgobj.data('speed'));

Not sure if it was just me but that worked a lot better.

Also, needed the ability to add a little offset so the image displays in the position I would like it to show. I added an offset field that lets you control it.

Copy link
Contributor

@thejimbirch thejimbirch left a comment

Choose a reason for hiding this comment

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

In general, when Drupal exports yaml files, I believe that the children of items are in alphabetical order.
Their position in Drupal can be ordered with the weight option.

In this case, dependencies > config and content > field_name should be re-ordered.

@albertski
Copy link
Collaborator Author

Good point. I made those fixes.

@thejimbirch
Copy link
Contributor

Excellent! Chrissy and I will review in the morning. She handled the Javascript for this module.

Copy link
Contributor

@christine-harder christine-harder left a comment

Choose a reason for hiding this comment

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

The changes in xeno-hero.js look great. It is a lot cleaner.

Renders and sets xeno_offset field as Twig variable parallax_offset.
Changes data-offset attribute to use parallax_offset Twig variable.
Excludes xeno_offset field from content being rendered.
@thejimbirch
Copy link
Contributor

@albertski I made some changes to the template. Please review. If you approve, I will ask @christine-harder to merge the PR.

@albertski
Copy link
Collaborator Author

@thejimbirch Your changes look good and I tested and they worked.

Still not sure if you are required to set a default value like you do in php. I'm thinking you don't have to in Twig but jut incase created a question on Drupal Answers.

@thejimbirch
Copy link
Contributor

We got good feedback on Drupal Answers that our solution is valid.

@christine-harder Let me know when you have availability and we can complete this Pull Request.

@christine-harder christine-harder merged commit d9d58fe into xenomedia:master Aug 18, 2017
@christine-harder
Copy link
Contributor

Pull request is merged 👍

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