-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Christina/drag drop outline 3 #4605
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
Conversation
|
Here is the comment about is-collapsible (from @talbs) Issue: After working further on this branch, I noticed a bug that is caused by toggling the .is-collapsible class. In short, the arrow to the left of the item's title rotates when dragging. Looking at the styling, we will need this class to be consistently on every element that is collapsible in the UI (both the collapsed and expanded styling depend on it). We may have to go the road of removing transitions or some other method than dropping this needed class. |
cms/static/sass/views/_outline.scss
Outdated
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.
is it a bug?
|
@talbs please take a look at my latest changes. I just disable transition when dragging via |
|
I just found a bug we will need to address. Dragging a unit into or out of a subsection does not update the state of the subsection (I assume this is true for sections as well). For example, |
|
Regarding bok choy test coverage, it would certainly be nice to have. But my experience with bok choy and drag and drop has been that the tests are flaky (I wrote drag and drop tests for the container page). Also, there is no way to do drag and drop without ActionChains (and testeng would prefer us not use them-- https://edx-wiki.atlassian.net/wiki/display/~clytwynec/Bok+Choy+Best+Practices+Questions). Time permitting, I am willing to try a bok choy test (and in particular, I'd like it to cover the bug I noted above), but I do not plan to hold up this PR for that. |
|
@polesye, you rock! Both of those fixes are great and my local outline looks and behaves very well. Things look good from my perspective. 👍 |
|
@polesye I responded to your feedback (and @talbs request to not show content or splints at unit level). I think the only remaining issue is the bug that parent xblocks will need to refresh when drag parent source is different from drag parent target. Actually, both section and subsection xblocks will need to update for dragging units. If you have time tomorrow, can you look into this? I have very little development time due to all the PR reviews and scrummaster duties. |
|
@cahrens sure. |
|
@cahrens bug is fixed. Please review my code and verify that everything works well. |
|
@polesye Unfortunately that change doesn't completely fix it. It is only re-rendering the source parent view, but when you drag into a different parent, the target parent view needs to re-render also. For example,
Drag unit from #2 to #1. #2 should turn blue and #1 should turn black I'm thinking we will need to go to more of an eventing model (where the view listens to events fired by the drag and drop code and then refreshes the appropriate views). |
|
@cahrens please take a look at 98c9d96. |
|
@polesye seems to work. Any way to make it less "flashy"? You can definitely tell the rendering is happening twice. |
|
Aside from known issues, lgtm. 👍 |
|
I believe this is ready to merge (Leslie has given a thumbs-up). I will wait until tomorrow so we can see test runs. |
|
@cahrens Think you need these changes to fix flaky UnitPublishing bok-choy tests |
* adding back in drag and drop indicators to outline DOM * correcting visual display of drag action in outline UI * adjusting styling of outline drag and drop indicators * adjusting styling of drag and drop was-dropped state
|
👍 |
Drag and drop on new outline page.
@polesye I will address your review feedback noted in #4550 tomorrow.
Can you take a look at the issue @talbs mentioned concerning the is-collapsible class?