-
Notifications
You must be signed in to change notification settings - Fork 71
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
Use virtual-dom for rendering the interface #27
Conversation
92028dc
to
46241c9
Compare
@mtyaka 👍 conditional review notes are addressed. |
- "sh -e /etc/init.d/xvfb start" | ||
install: | ||
- "sh install_test_deps.sh" | ||
- "python setup.py develop" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would not catch things like assets that aren't included in the package, right? To catch this type of issues with Travis, can you install the package instead of linking directly to the repository, like in https://github.com/open-craft/xblock-mentoring/blob/master/.travis.yml#L12 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a while to realize what you mean, but I think I get it now. This just ensures that the build fails if you forget to include some directories in the package_data
in setup.py
, but assuming your package_data
is correct, there should be no difference, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtyaka Yup, exactly.
46241c9
to
3f71b0a
Compare
This patch also includes a change where the items are positioned absolutely (rather than relatively) after dropping into the correct zone, which means that after dropped into the zone, the element no longer occupies vertical space in the item list on the left, which is desired behaviour.
Also includes Travis CI integration.
3f71b0a
to
d1e4d12
Compare
state = { | ||
'top': attempt['top'], | ||
'left': attempt['left'], | ||
'absolute': True # flag for backwards compatibility (values used to be relative) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding a comment too, here
@mtyaka Changes looks good! 👍 I still have the empty preview issue I described above though, upon adding a drag and drop the first time. It works well on reload and in apros, so I'm going to merge now to send to QA, but could you have a look? |
Use virtual-dom for rendering the interface
@antoviaque Thanks! The empty preview was supposed to be fixed with the last update. I just tried again and the preview loads fine after adding a new drag and drop block to the page for me, no need to reload. |
@mtyaka Might just have been me then - I might not have correctly reloaded the latest version - thanks for checking! |
@antoviaque @mtyaka adding drag-n-drop for the first time shows normal preview for me. |
@e-kolpakov Cool, thanks for the confirmation! |
This patch rewrites the JS code to use virtual-dom rather than ad-hoc jQuery element manipulation for rendering the interface.
It also includes a change where the items no longer occupy space in the list on the left after they are dropped to the correct zone.