Skip to content

feat: PREVENT_DEFAULT command #187

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.

Sign up for GitHub

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

Jump to bottom
Merged
michaelobriena merged 1 commit into from
Jun 11, 2015
Merged

feat: PREVENT_DEFAULT command #187

michaelobriena merged 1 commit into from
Jun 11, 2015

Conversation

@alexanderGugel
Copy link
Member

@alexanderGugel alexanderGugel commented May 26, 2015

  • Add PREVENT_DEFAULT and PROMOTE_DEFAULT commands
  • No longer preventDefault by default
  • Add UNSUBSCRIBE command

Fixes #167
@DnMllr @browles Thoughts?

michaelobriena
michaelobriena reviewed May 28, 2015
View reviewed changes
dom-renderers/DOMRenderer.js Outdated
Copy link
Member

@michaelobriena michaelobriena May 28, 2015

Choose a reason for hiding this comment

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

Needs docs

@alexanderGugel
Copy link
Member Author

alexanderGugel commented May 28, 2015

@michaelobriena I mentioned the missing docs in the PR. My question was rather whether we want a preventDefault command or if we should handle that case in addUIEvent. E.g. @browles suggested having an options argument in addUIEvent instead.

Counterargument: preventDefault is DOM-specific and adding it to addUIEvent makes dynamically changing preventDefault after adding the event impossible.

Thoughts?

@michaelobriena
Copy link
Member

michaelobriena commented May 28, 2015

I know you did, it is just a way for me to note it for myself :)

@alexanderGugel
Copy link
Member Author

alexanderGugel commented May 28, 2015

Ahhh ok :)

@browles
Copy link
Contributor

browles commented May 28, 2015

How about just .allowDefault?

@alexanderGugel alexanderGugel force-pushed the preventDefault-optional branch 2 times, most recently from cdd359b to 8992545 Compare May 28, 2015 20:23
@alexanderGugel
Copy link
Member Author

alexanderGugel commented May 28, 2015

@browles 👍 changed

@alexanderGugel alexanderGugel force-pushed the preventDefault-optional branch from 8992545 to d869275 Compare May 28, 2015 21:09
@michaelobriena michaelobriena added this to the 0.6.0 milestone Jun 4, 2015
@alexanderGugel alexanderGugel force-pushed the preventDefault-optional branch from d869275 to f56d1a7 Compare June 5, 2015 16:54
@FarhadG
Copy link
Contributor

FarhadG commented Jun 8, 2015

What's the status on this PR?

@alexanderGugel @michaelobriena

@alexanderGugel
Copy link
Member Author

alexanderGugel commented Jun 8, 2015

This should be merged ASAP IMO.

@alexanderGugel alexanderGugel mentioned this pull request Jun 8, 2015
Closed
@solomon-gumball
Copy link
Contributor

solomon-gumball commented Jun 9, 2015

Don't these conflict? #215

@alexanderGugel
Copy link
Member Author

alexanderGugel commented Jun 9, 2015

Yes, they do. Why did/ Did you copy over "UNSUBSCRIBE" instead of basing off of this PR?

@alexanderGugel alexanderGugel force-pushed the preventDefault-optional branch from f56d1a7 to 8133f65 Compare June 9, 2015 19:48
@solomon-gumball
Copy link
Contributor

solomon-gumball commented Jun 9, 2015

Didn't copy anything over, just started from scratch.

We can merge this and then I can reconcile the other PR with it.

@alexanderGugel
Copy link
Member Author

alexanderGugel commented Jun 9, 2015

Sounds good. Ok.

@michaelobriena
Copy link
Member

michaelobriena commented Jun 10, 2015

@alexanderGugel This is not ready to merge as there are still public functions without docs.

@alexanderGugel
Copy link
Member Author

alexanderGugel commented Jun 10, 2015

NOOOO!! Ok. All user-facing APIs are documented. Will fix.

@alexanderGugel
feat: PREVENT_DEFAULT command
fe0e330 
* Add `PREVENT_DEFAULT` and `PROMOTE_DEFAULT` commands
* No longer preventDefault by default
* Add `UNSUBSCRIBE` command
@alexanderGugel alexanderGugel force-pushed the preventDefault-optional branch from 8133f65 to fe0e330 Compare June 10, 2015 09:47
@alexanderGugel
Copy link
Member Author

alexanderGugel commented Jun 10, 2015

@michaelobriena Sorry. Thought the diff was outdated. All documented now.

@michaelobriena michaelobriena merged commit fe0e330 into Famous:develop Jun 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug cleanup Ready To Merge

Projects

None yet

Milestone

0.6.0

Development

Successfully merging this pull request may close these issues.

5 participants

@alexanderGugel @michaelobriena @browles @FarhadG @solomon-gumball