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

Fix mousemove bug, ghost offset bug, requiring children if no handle … #16

Merged
merged 5 commits into from
Sep 29, 2017

Conversation

jvanbruegge
Copy link
Collaborator

…option set, remove need for emitBetween, use border-box, add code to implement ghostClass options

Continuation of #15

…option set, remove need for emitBetween, use border-box, add code to implement ghostClass options
@ntilwalli
Copy link
Contributor

ntilwalli commented Aug 10, 2017

@jvanbruegge I just tried the manyFixes branch and it works for me. I have the selectionDelay option set to non-zero for the simple example, which means you need to keep mousedown for a certain period of time before the selection becomes active. I tested with the simple and horizontal examples. Which example did you test?

@ntilwalli
Copy link
Contributor

Also I needed to switch to @types/handlebars to v4.0.33 due to this issue: DefinitelyTyped/DefinitelyTyped#18025

@jvanbruegge
Copy link
Collaborator Author

The parentSelector example has a weird offset and the update example throws an error

Also I needed to switch to @types/handlebars

We are not using handlebars?

@ntilwalli
Copy link
Contributor

ntilwalli commented Aug 10, 2017

The reason the updateEvent example is crashing is because the selectionDelay default value is missing. Seems that code change was skipped during the merge. You can see the additional line in my local branch: https://github.com/ntilwalli/cyclejs-sortable/blob/master/src/helpers.ts#L25

@ntilwalli
Copy link
Contributor

Also the @types/handlebars seems to be required by typedoc. I did a fresh npm install and it pulled in 4.0.35 which caused the build to fail. Overriding that lib to 4.0.33 allowed the build to work... dunno

@jvanbruegge
Copy link
Collaborator Author

No, that line is also in the merge: https://github.com/cyclejs-community/cyclejs-sortable/pull/16/files#diff-78060b682ecec44974ea9e5dde4a8310R36

)
.take(1)
.of(ev)
.compose(delay(options.selectionDelay))
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo/bug. The options.selectionDelay should read defaults.selectionDelay

Copy link
Contributor

Choose a reason for hiding this comment

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

Changing that line will fix the issue in the updateEvent example

Copy link
Contributor

Choose a reason for hiding this comment

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

The whole line should read .compose(delay(defaults.selectionDelay))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Weird that typescriot did not complain

Copy link
Contributor

Choose a reason for hiding this comment

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

TS didn't complain because an 'options' variable exists in the scope. 'defaults' is the normalized version of 'options' which contains the defaults.

@ntilwalli
Copy link
Contributor

ntilwalli commented Aug 10, 2017

Regarding the weird offsets, the parent elements all need to have position: relative added to their css styles. That applies to both the updateEvent and parentSelector examples. Would you like me to make the updates (including the selectionDelay issue) and resubmit a PR?

@jvanbruegge
Copy link
Collaborator Author

Please make a Pr based on this one, then I can squash&merge

@ntilwalli
Copy link
Contributor

#17

@jvanbruegge jvanbruegge merged commit fb2b2e7 into master Sep 29, 2017
@jvanbruegge jvanbruegge deleted the manyFixes branch September 29, 2017 09:24
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.

2 participants