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

Allow targeting of non-body elements #21

Closed
wants to merge 4 commits into from
Closed

Allow targeting of non-body elements #21

wants to merge 4 commits into from

Conversation

dabernathy89
Copy link

This is my first time contributing to a project, so forgive me if I'm going about this incorrectly. I liked your plugin, but I really only needed to run the slideshow on my header. I made some changes to the code to allow it to use a selector other than 'body', although body remains the default.

introduce new option bgtarget
replace instances of 'body' with bgtarget
change from fixed to absolute positioning
moved position of bgtarget to end of options object literals
passed the setting through when slideshow calls $.vegas
@jaysalvat
Copy link
Owner

Thanks for your contribution.
I will check that as soon as possible. Be patient I'm really overbooked currently.
Thanks again!

@wiseguydigital
Copy link

Hey dabernathy89, this is just to say that I took a look at the code and think you will need to add a new var of $bgTarget = $() & set it in init() rather than using options.bgtarget throughout. options will not be defined in some of the methods.

@dabernathy89
Copy link
Author

I did end up passing options to some of the methods that didn't have it previously. You think it'd be better to avoid that? With your suggestion, how would the user set the target?

@wiseguydigital
Copy link

Personally I would add it to the class config and set the value in init(). By adding it as an option to say overlay() you are hinting that the overlay could be set on a separate element to the main vegas element. I think you want one master target element and that's it. What do you reckon?

@jaysalvat
Copy link
Owner

Hi there ! Thanks for contributing.

To keep the jQuery convention, it would be great to have a $(elm).vegas(options) to apply vegas to any element.

@wiseguydigital
Copy link

I have opened a pull request with the jQuery wrapper style: #23

@dabernathy89
Copy link
Author

Nice! Better solution than mine.

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