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

#18 custom script builder #30

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

#18 custom script builder #30

wants to merge 2 commits into from

Conversation

dmtri
Copy link
Contributor

@dmtri dmtri commented Sep 19, 2021

No description provided.

@dmtri
Copy link
Contributor Author

dmtri commented Sep 19, 2021

@manhtai please take a look, if everything is okay then I will polish the UI and finalize this MR

@manhtai
Copy link
Member

manhtai commented Sep 19, 2021

@dmtri The MR looks great, I have some points to discuss though:

  • Should we add another state (edit mode / view mode) to hide the function source code, because most of the time we needn't see it?
  • Should we watch the change of input and apply the function immediately instead of hitting "Run" every time?

Again, nice work 💪

@dmtri
Copy link
Contributor Author

dmtri commented Sep 19, 2021

@manhtai

  1. Yes, I agree
  2. I'm not sure about this. It might crash the app when users are typing incomplete/erroneous code and they don't intend to run it yet. I could handle Enter keypress to save mouse movements probably :D
    Cheers!

@manhtai
Copy link
Member

manhtai commented Sep 19, 2021

@dmtri

  1. I meant when in view mode, a wrong function / argument will output an error message anyway. No way it will crash the app 🤔

@dmtri
Copy link
Contributor Author

dmtri commented Sep 19, 2021

@manhtai I see, watching argument changes in view mode is reasonable but watching function changes in render mode feels kinda wrong to me (user might not want auto run on every change) 😅

@manhtai
Copy link
Member

manhtai commented Sep 19, 2021

@dmtri Yes, when in edit mode we shouldn't let the function run with every change, but when in view mode it is reasonable, just like every other built-in one ;)

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

2 participants