search

oozou / slate-suggestions

68 stars 26 forks source link

More efficient opening/closing of Portal #17

Open oyeanuj opened 7 years ago

oyeanuj commented 7 years ago

Hi @aunsuwijak, thank you for the very helpful library!

While playing with the library I noticed that there were two regex checks being made - one in the plugin code and then again in the Portal code.

So, it seems that the Portal is essentially making its own decision to stay open or close (by removing the style attribute without checking if it is open in adjustPosition) on every change of state.

So, my conceptual question is if there is a more efficient/performant way where the regex is being checked once which determines if the portal is open or not, and closes only if open?

Let me know if I've misunderstood the logic somewhere!

cc: @bkniffler since his PR #15 touches upon this and he mentioned some thoughts about a refactor.

aunswjx commented 7 years ago

Thanks @oyeanuj

Actually, the reason I put a regex check in the plugin is when the regex matches some of behavior of onKeyDown method should be changed e.g. Up/Down arrow. Only one behavior that related to open/close portal is I manually call closePortal method when enter key is pressed.

BTW, I think we can assign some state to the portal to indicate the plugin that the portal is open or close instead of checking the regex we can use that state to determine onKeyDown behavior.

What do you think?

oyeanuj commented 7 years ago

Thanks for the quick response, I see what you mean. However, one follow-up:

use that state to determine onKeyDown behavior

Do you mean the onKeyDown handler in the plugin? If yes, how would it access the state inside the portal?

aunswjx commented 7 years ago

@oyeanuj Yes.

Currently there is a callback object which defined in the plugin and passed to the portal and provide method like closePortal back to the plugin. I think we can use the same technique to pass the value from the portal back to the plugin.

oyeanuj commented 7 years ago

@aunsuwijak Take a look at 0.18 release for Slate. It has a new method for renderPortal in a plugin that might be a cleaner pattern. There is an example is https://github.com/ianstormtaylor/slate/issues/635 which I think might be relevant to this library.

aunswjx commented 7 years ago

@oyeanuj Very nice, thanks.

Will take a look. I think this can improve this plugin a lot since it was created when slate is around 0.12 or 0.13.

Anyway, thanks a lot.

oyeanuj commented 7 years ago

@aunsuwijak Just checking in to see if you've had a chance to revisit this?

aunswjx commented 7 years ago

@oyeanuj I really want to make some progress on this plugin, but currently I can't cuz I have some other work to do so. I don't want this library to be freeze, so I'm happy if somebody or you can try contribute to it. Sorry, for didn't make any improvement for along time.

oyeanuj commented 7 years ago

@aunsuwijak thanks for the response. i'll try to take out some time and contribute as well, once I am working on mentions.