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

Introduction to moveable mixin #209

Merged
merged 9 commits into from
Jun 9, 2016
Merged

Introduction to moveable mixin #209

merged 9 commits into from
Jun 9, 2016

Conversation

cmwd
Copy link
Contributor

@cmwd cmwd commented May 28, 2016

This is a part of #208.
Previously we were using jquery-ui draggable in order to move objects.

In future we should refactor this mixin to use css transform property instead of top/left.
This is why:
https://csstriggers.com/left vs https://csstriggers.com/transform

This is a part of komitywa#208.
Previously we were using jquery-ui draggable in order to move objects.

In future we should refactor this mixin to use css transform property instead of top/left.
This is why:
https://csstriggers.com/left vs https://csstriggers.com/transform
@codecov-io
Copy link

codecov-io commented May 28, 2016

Current coverage is 20.95%

Merging #209 into master will increase coverage by 3.95%

@@             master       #209   diff @@
==========================================
  Files            19         20     +1   
  Lines           400        420    +20   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits             68         88    +20   
  Misses          332        332          
  Partials          0          0          

Powered by Codecov. Last updated by bb06e1a...d718a78

@cmwd
Copy link
Contributor Author

cmwd commented May 31, 2016

I updated the code to use transform instead of left/top.

before:
screen shot 2016-05-31 at 22 04 12

after:
screen shot 2016-05-31 at 22 05 41

🐎 🚗 💨 🚤 ✈️ 🐢

@magul
Copy link
Contributor

magul commented May 31, 2016

These diagrams looks great, much like these created by Sam :) I'm starting review, so be aware!

@@ -0,0 +1,50 @@
export const MOVE_END = 'moveend';
export const MOVE_START = 'movestart';
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary export - we don't MOVE_START anywhere else

@magul
Copy link
Contributor

magul commented May 31, 2016

Ok, I've review it. Looks like really good job! 🚀

I have some doubts if mixin here will not be an overkill (only these objects will be draggable). And You haven't removed jquery-ui from import and from package.json.

@cmwd
Copy link
Contributor Author

cmwd commented Jun 1, 2016

And You haven't removed jquery-ui from import and from package.json.

We still need jquery-ui for drag/drop. This is only part of the issue. I wanted to split it into two pieces because of PR size.

I have some doubts if mixin here will not be an overkill (only these objects will be draggable).

Yeah, You're probably right. But I like that idea of creating separated mixins for features. Even though we use it for single element. It makes code easier to test. We might keep this code inside object module. What do you think?

@cmwd
Copy link
Contributor Author

cmwd commented Jun 1, 2016

I found two major bugs in implementation:

  1. I'm creating one scope for all instances,
  2. Because I'm assign mousemove to every element it creates collision.

I should fix it till end of the week.

@cmwd
Copy link
Contributor Author

cmwd commented Jun 7, 2016

@magul I made some major changes comparing to last version. I thought again about the way I implemented this and I didn't like it so much.
First of all I wanted to provide well encapsulated component. So I created simple function where I'm passing view instance. This way We don't need to deal so much with this, extending objects and creating pseudo private variables in Backbone classes.
Also I tested again whole thing and came out that dragging has some issues. For example when I dragged over another element. To solve it I changed the way We listening for mouseup and mousemove - now these events are global and assigned dynamically.
I changed the way we are binding mousedown. This way We don't need to reassign events.

@@ -0,0 +1,56 @@
/* global describe,beforeEach,afterEach,before,it:false */
Copy link
Contributor

Choose a reason for hiding this comment

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

what false at the end here is doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means that these methods are read only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we could improve this. Probably eslint has some environment settings for mocha/chai.

@magul
Copy link
Contributor

magul commented Jun 8, 2016

Few questions, but look ok. 🚀

@magul magul merged commit f885f6b into komitywa:master Jun 9, 2016
@cmwd cmwd deleted the issue208 branch June 9, 2016 22:36
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.

3 participants