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

Add cursorDidChange callback to editor compo #148

Merged
merged 2 commits into from
Jun 27, 2018

Conversation

mattmcmanus
Copy link
Contributor

We've run into issues where the inputModeDidChange callback wasn't firing at times when we wanted it to. cursorDidChange seems to do exactly what we want. We're implementing an interface that shows a link tooltip/prompt when you click on the link, rather than hover on it. I saw that you removed this in #69, so I'm not sure if you want it back or not.

I'm up for discussion about how to implement and test this. We're doing most of our work in a subclass of mobiledoc-editor and so in this PR it's just setting up the callback and an empty method.

Here is what our interface is looking like at the moment:
mobiledoc-editor-tooltip

@bantic
Copy link
Contributor

bantic commented Mar 2, 2018

@mattmcmanus Thanks for this PR. This makes sense to me to add. It would be nice not to have to punch through all these editor hooks directly (to make this addon easier to keep up-to-date with mobiledoc-kit changes), but that's definitely the pattern we have so far. Adding cursorDidChange is a good idea.

I am going to work on fixing the CI builds separately and return to merge this when I've sorted that out.

@mattmcmanus
Copy link
Contributor Author

That's a good thought. It would be pretty straightforward to wire up if kit's editor.js exported it's callback queue constants. The overlap in lifecycle names between it and ember's lifecycle hooks add some cognitive overhead. Maybe the callback method names are prefixed with editor? So the ember component has some setup code that runs after the editor is instantiated. It would loop over the exposed callback queues and setup a simple callback method that checks if a prefixed method exists (ie: editorCursorDidChange) and runs it.

Is that along the lines of what you're thinking?

@bantic
Copy link
Contributor

bantic commented Jun 1, 2018

@mattmcmanus Sorry for the delay. I've fixed the build and I restarted the build for this PR. It might need a rebase, but if it goes green now I'll merge it straight away.

@bantic
Copy link
Contributor

bantic commented Jun 1, 2018

@mattmcmanus Would you mind doing a rebase off master when you can?

@mattmcmanus
Copy link
Contributor Author

@bantic Rebased.

@jasonbekolay
Copy link
Contributor

@bantic I've rebased this and fixed the test failure

@bantic
Copy link
Contributor

bantic commented Jun 27, 2018

@jasonbekolay and @mattmcmanus thank you both!

We should also be sure to document this so that future travelers are aware of it. I'll add an issue.

@bantic bantic merged commit 39c573c into bustle:master Jun 27, 2018
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.

None yet

3 participants