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

Deprecated widgy.backbone.render #268

Open
wants to merge 1,098 commits into
base: master
Choose a base branch
from

Conversation

zmetcalf
Copy link
Contributor

Beat you to it @rockymeza. This is for issue #259.

acatton and others added 30 commits August 14, 2013 16:23
BeautifulSoup import is the third version, which was a dependency of
django-fusionbox.
This middleware have meen moved to
git://github.com:fusionbox/django-fusionbox.git@f6147ab
merged fusionbox#98

* no-fusionbox:
  Get rid of django-fusionbox in the documentation
  Remove fusionbox.middleware.GenericTemplateFinderMiddleware dependency
  Add django argonauts dependency
  Use widgy queryset on ReviewedVersionCommit
  Use BeautifulSoup 4
  Get rid of fusionbox_tags dependency
  Switch from django-fusionbox to django-argonauts
  Copy PhoneNumberField into widgy
  Add phonenumbers dependency to setup.py for PhoneNumberField
  Remove dependency from django-fusionbox QuerySetManager
  Remove fusionbox.core from the demo site INSTALLED_APPS
  Get rid of fusionbox.behaviors dependency
  Remove widgy.contrib.replacements module
  Remove django-fusionbox from requirements.txt file
  Remove fusionbox internal fabfile
  Move requirements file in demo site
  Remove django-fusionbox dependency in setup.py
Until Mezzanine supports Django 1.6
…ome existing logic to include new ViewList methods
PhoneNumberField unittest - isolated on class
Form redirection was broken:

    * If the form success page doesn't have a “from” parameter, the
    success page raises an Error.
    (Fixed with the try...except KeyError)

    * If the “from” parameter is relative, or if it is something
    like '"><img src="'. It raises a NoReverseMatch, because Django
    thinks it's a module, and tries to import it. This could lead to a
    remote code execution if a user succeed to put a __init__.py file in
    the /media/ directory and succeed to import it.
    (Fixed by using HttpResponsePermanentRedirect instead of redirect() )

    * If the “from” parameter is “http://example.com” widgy will
    redirect to example.com, allowing somebody to give links like:
    <http://good.example.com/form/success/?from=http://evil.example.com/>.
    <http://evil.example.com/> can be fully URL encoded in order to
    hide it.
    (Fixed by using is_safe_url)
_.each(this.cssClasses(), this.$el.addClass, this.$el);

return this;
// Depricated for renderPromise
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: deprecated

@rockymeza
Copy link
Contributor

Hey @zmetcalf,

I like it. I just had some comments in there. Let's get these addressed.

Are there any calls to render at all now? If not, we could just remove all of the render methods. That way we can just get rid of the dead code. thoughts?

@zmetcalf zmetcalf force-pushed the deprecate_render branch 2 times, most recently from a0a06bf to ac4f4c3 Compare October 26, 2014 18:03
@zmetcalf
Copy link
Contributor Author

@rockymeza I changed the name of shelf_to_el to shelf_promise. Also, I agree with removing all the render methods because it is no longer used and backbone's implementation is a no-op. Also, I just took out the render method of templates.js, can you please review? I did not see any side effects because it was never called anyway.

As for returning the promise with the .then attached in shelf, it does not work. The original promise object is different than the promise.then(... .

});

shelf.resizeShelf();
$(window).resize(shelf.resizeShelf);
Copy link
Contributor

Choose a reason for hiding this comment

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

right here you need to return shelf.

Copy link
Contributor

Choose a reason for hiding this comment

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

you need to return shelf here. Then you can just return promise.then

@rockymeza
Copy link
Contributor

@zmetcalf, there are several places where you need to call done() to make sure that errors get thrown.

@zmetcalf
Copy link
Contributor Author

@rockymeza, thank you for the help with that. All fixed up I hope!

@rockymeza
Copy link
Contributor

merged in 638453e

@rockymeza rockymeza closed this Nov 5, 2014
@rockymeza
Copy link
Contributor

Hey Zach, I unmerged this (seeing as it was only merged for 4 minutes).

I found a bug. This broke the visible_drop_targets optimization. I will investigate.

@rockymeza
Copy link
Contributor

@zmetcalf,

I added a commit, you can see it here: https://github.com/fusionbox/django-widgy/tree/deprecate_render

It fixes two bugs that came out of the deprecate render commit:

  1. Tabbed Component overrides createDropTarget which was changed to return a promise, I just updated it.
  2. The addDropTargets changes broke the visible_drop_targets optimization. I almost didn't notice this, but thanks to the debug statements that @gavinwahl had in the code, I was able to notice it. The problem was that refreshDropTargetVisibility was being called too early. It needs to be called after all of the drop targets have been rendered and added to the DOM. To fix that, I wrapped them up in a Q.all call and then called refreshDropTargetVisibility when they all finished.

Can you review my changes and then we'll merge?

@rockymeza rockymeza reopened this Nov 5, 2014
@zmetcalf
Copy link
Contributor Author

zmetcalf commented Nov 5, 2014

Wow, good catch! The fix worked on my end.

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

6 participants