Skip to content
This repository has been archived by the owner on Oct 8, 2024. It is now read-only.

Possibility to use existing wrapper (parent) element. #51

Closed
wants to merge 2 commits into from

Conversation

lkrzyzanek
Copy link

Fixes #50

$(options.element).addClass(options.className).css(options.styles)
);
this.element.css(options.innerElementStyles);
if (!options.useExistingParent) {

Choose a reason for hiding this comment

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

Instead of introducing useExistingParent to this already large set of options, could this be simplified by checking the value of the wrapper.element option and only performing the wrap if it's truthy? Then, if you wanted to omit the wrapper element, you could simply pass a falsy value for that option.

Copy link
Author

@lkrzyzanek lkrzyzanek Apr 13, 2017

Choose a reason for hiding this comment

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

Yeah it's good idea to use wrapper.element to either create a wrapper if it's string value e.g. <div> or just use it if it's jquery pointer to an DOM object. Let me redesign this next week. Thanks.

@lkrzyzanek
Copy link
Author

@erikjung redesigned. I used options.element.jquery to check if it's real jquery element or not. Please review again.

);
this.wrapperElement = this.element.parent();
this.element.css(options.innerElementStyles);
if (!options.element.jquery) {
Copy link
Member

Choose a reason for hiding this comment

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

jQuery elements are also valid jQuery selectors. So instead of checking if the selector is a jQuery object, we could instead check to make sure it's not already the parent element before wrapping.

Example:

// Keep this separate, as you've already done
this.element.css(options.innerElementStyles);
// Create the wrapper element, using `options.element`
this.wrapperElement = $(options.element).addClass(options.className).css(options.styles);
// If this wrapper element doesn't already have the element as a descendent...
if (!this.wrapperElement.has(this.element).length) {
  // ...then wrap the descendent and override the local reference
  this.element.wrap(this.wrapperElement);
  this.wrapperElement = this.element.parent();
}

@tylersticka
Copy link
Member

Closing this PR due to lack of activity.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possibility to use existing wrapper (parent) element
3 participants