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

Add support for creating instances by dropping objects from objects list #650

Merged
merged 10 commits into from Sep 19, 2018

Conversation

Projects
None yet
4 participants
@4ian
Owner

4ian commented Sep 16, 2018

Add drag'n'drop from objects list.

  • Should be more intuitive than selecting object then making one click.
  • This will allow the blue selection background to be used for highlighting the object in #649
  • Tested on Chrome and Firefox.

Ideally SceneEditor/index.js should be flow typed as it's becoming large and this could avoid regressions/mistakes.

@4ian

This comment has been minimized.

Show comment
Hide comment
@4ian

4ian Sep 16, 2018

Owner

@blurymind Are you able to test this branch? If yes, let me know if that's working for you.

dragdrop

What I recommend is that we merge this first, then you can update your PR to have the highlight of the selected object.

Owner

4ian commented Sep 16, 2018

@blurymind Are you able to test this branch? If yes, let me know if that's working for you.

dragdrop

What I recommend is that we merge this first, then you can update your PR to have the highlight of the selected object.

@zatsme

This comment has been minimized.

Show comment
Hide comment
@zatsme

zatsme Sep 16, 2018

Looking at your GIF it seems to me that you need to drag only the sprite otherwise it's not obvious where it's going to drop. Yes, I know you can move it after... but it looks wrong.

zatsme commented Sep 16, 2018

Looking at your GIF it seems to me that you need to drag only the sprite otherwise it's not obvious where it's going to drop. Yes, I know you can move it after... but it looks wrong.

@blurymind

This comment has been minimized.

Show comment
Hide comment
@blurymind

blurymind Sep 16, 2018

Contributor

This will be very very good for touch interfaces imo. Its definitely more intuitive to add instances like that.

I think @zatsme means that because it is still changing the order in the list, while the drag is outside of the list- that is a bit distracting

Contributor

blurymind commented Sep 16, 2018

This will be very very good for touch interfaces imo. Its definitely more intuitive to add instances like that.

I think @zatsme means that because it is still changing the order in the list, while the drag is outside of the list- that is a bit distracting

@4ian

This comment has been minimized.

Show comment
Hide comment
@4ian

4ian Sep 17, 2018

Owner

I know it's a bit distracting but I've not been able to find how to prevent this :/

Owner

4ian commented Sep 17, 2018

I know it's a bit distracting but I've not been able to find how to prevent this :/

@blurymind

This comment has been minimized.

Show comment
Hide comment
@blurymind

blurymind Sep 17, 2018

Contributor

It seems to work great :) I tested it and changed my pull so it compliments this one.

Unfortunately I think that it duplicated some of the changes in this one. Hope its not a problem

Contributor

blurymind commented Sep 17, 2018

It seems to work great :) I tested it and changed my pull so it compliments this one.

Unfortunately I think that it duplicated some of the changes in this one. Hope its not a problem

@blurymind

This comment has been minimized.

Show comment
Hide comment
@blurymind

blurymind Sep 17, 2018

Contributor

I can resubmit it after you merge this one if it is a problem
Edit- re-submitted here #651

Contributor

blurymind commented Sep 17, 2018

I can resubmit it after you merge this one if it is a problem
Edit- re-submitted here #651

@Bouh

This comment has been minimized.

Show comment
Hide comment
@Bouh

Bouh Sep 17, 2018

Contributor

To avoid moving in the list, there can be a timer of 0.5s on the touch, if it is below we put on the scene, and above we move in the list ?

Contributor

Bouh commented Sep 17, 2018

To avoid moving in the list, there can be a timer of 0.5s on the touch, if it is below we put on the scene, and above we move in the list ?

@4ian

This comment has been minimized.

Show comment
Hide comment
@4ian

4ian Sep 17, 2018

Owner

It's more complicated, unfortunately. The reordering by drag'n'drop is done by the list. When dragging an object, this drag'n'drop is activated.
I cannot deactivate it without hacks that are super dirty and still breaking in even more nasty ways.

I'm thinking that we should almost have no reordering by drag'n'drops of lists by default, and have an option in context menu called "Reorder". When clicked, the list switch to reordering (with drag handles on the left, like the list of layers) and things can be drag'n'dropped (only in the list).
When reordering is not activated, drag'n'drop would only work on the scene (and for other list, would do nothing).

What do you think? It's a pattern that is commonly used on mobile (you activate an option showing drag handle on the left of items in a list, that you can move). But it would requires reworking quite a lot of lists (resources editor, groups, objects list) so that it's consistent everywhere.

Owner

4ian commented Sep 17, 2018

It's more complicated, unfortunately. The reordering by drag'n'drop is done by the list. When dragging an object, this drag'n'drop is activated.
I cannot deactivate it without hacks that are super dirty and still breaking in even more nasty ways.

I'm thinking that we should almost have no reordering by drag'n'drops of lists by default, and have an option in context menu called "Reorder". When clicked, the list switch to reordering (with drag handles on the left, like the list of layers) and things can be drag'n'dropped (only in the list).
When reordering is not activated, drag'n'drop would only work on the scene (and for other list, would do nothing).

What do you think? It's a pattern that is commonly used on mobile (you activate an option showing drag handle on the left of items in a list, that you can move). But it would requires reworking quite a lot of lists (resources editor, groups, objects list) so that it's consistent everywhere.

@blurymind

This comment has been minimized.

Show comment
Hide comment
@blurymind

blurymind Sep 17, 2018

Contributor

The reorder move while drag to drop doesnt bother me that much personally. If its causing so much refactoring everywhere is it really worth investing time in it over other things? :)

Contributor

blurymind commented Sep 17, 2018

The reorder move while drag to drop doesnt bother me that much personally. If its causing so much refactoring everywhere is it really worth investing time in it over other things? :)

@Bouh

This comment has been minimized.

Show comment
Hide comment
@Bouh

Bouh Sep 18, 2018

Contributor

You talk about handles as for layers why not put this handle by default, then the other part would be used for the slide placed in the scene.

Because a button checked to activate the move is one more button. The fewer clicks, the simpler it is, the more visual, the simpler it is too.

Otherwise I agree with blurymind, if this is too complicated let's leave it as it is, it doesn't bother too much, let's wait for user feedback to change if it doesn't work.

Red : move in list
BLue drag and drop on scene
idea

Contributor

Bouh commented Sep 18, 2018

You talk about handles as for layers why not put this handle by default, then the other part would be used for the slide placed in the scene.

Because a button checked to activate the move is one more button. The fewer clicks, the simpler it is, the more visual, the simpler it is too.

Otherwise I agree with blurymind, if this is too complicated let's leave it as it is, it doesn't bother too much, let's wait for user feedback to change if it doesn't work.

Red : move in list
BLue drag and drop on scene
idea

@4ian

This comment has been minimized.

Show comment
Hide comment
@4ian

4ian Sep 18, 2018

Owner

@Bouh Yes your mockup looks right, that would be like this. I was just concerned that this was taking too much space on screen.

Anyway for now I'll let this as is, as it's still an improvement over the current situation (even if it's a bit disturbing to see the objects moves when you drag'n'drop, should not be too much of a problem).
Will rework the list later :)

Owner

4ian commented Sep 18, 2018

@Bouh Yes your mockup looks right, that would be like this. I was just concerned that this was taking too much space on screen.

Anyway for now I'll let this as is, as it's still an improvement over the current situation (even if it's a bit disturbing to see the objects moves when you drag'n'drop, should not be too much of a problem).
Will rework the list later :)

@4ian 4ian merged commit 6cd9e4b into master Sep 19, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details

@4ian 4ian deleted the feature/drop-instance-from-objects-list branch Sep 19, 2018

@blurymind

This comment has been minimized.

Show comment
Hide comment
@blurymind

blurymind Sep 20, 2018

Contributor

Thank you @4ian . This is an awesome usability addition :D

Contributor

blurymind commented Sep 20, 2018

Thank you @4ian . This is an awesome usability addition :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment