You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by zhongneu <gi...@git.apache.org> on 2016/02/13 07:22:51 UTC
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
GitHub user zhongneu opened a pull request:
https://github.com/apache/incubator-zeppelin/pull/713
Add checkbox as a type of dynamic forms
First attempt to add checkbox support for dynamic forms. There are several concerns:
1. I use Input.type to distinguish between "input", "selection" and "checkbox" forms, but I am not sure whether Input.type has already been used by any other places
2. Changed the way of detecting type of forms, which is not backward compatible. This is probably not a problem, because the notebook will be migrated into new version after the user run the paragraph
3. I think I need some guidance on the style sheet (I am a super new web developer). Currently it is quite ugly
TODOs:
- [ ] Compatibility test with notes from previous versions
- [ ] Documentation for checkbox
- [ ] Nice CSS layout
Also fixed a bug in this pull request: ZEPPELIN-669
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zhongneu/incubator-zeppelin dynamic-forms-checkbox
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-zeppelin/pull/713.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #713
----
commit 6969e8ca38ab5aa1370bee3cf792d8d639bdec2b
Author: Zhong Wang <zh...@zhongs-macbook-pro.local>
Date: 2016-02-13T04:52:37Z
first attempt of adding checkbox to dynamic forms
commit e190707c15629d25d00cdc1be5e757d2564a26d1
Author: Zhong Wang <wa...@gmail.com>
Date: 2016-02-13T06:09:38Z
fix several bugs, including ZEPPELIN-669
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by zhongneu <gi...@git.apache.org>.
Github user zhongneu commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-183800463
I've submitted one more commit with better layout and supporting show/hide options. I do run into an issue: the show/hide option will not be saved. Currently, I use formulaire.hidden to store the status, but it won't be committed because it is not part of the COMMIT_PARAGRAPH protocol.
I am not sure about what is the best way to add the form hidden status into the protocol. Any suggestions?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by zhongneu <gi...@git.apache.org>.
Github user zhongneu commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-183713630
@doanduyhai The current parsing code supports syntax like: ${label:type=...}. I do have concer about this, because from the code I cannot see what does this type refer to. Is it the type of the value, or the type of the form?
I don't see the type is used anywhere in the current code though, but I am not sure whether it is used outside of the project (3rd-party interpreters)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by zhongneu <gi...@git.apache.org>.
Github user zhongneu commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-184010942
Agreed. I will revoke the changes of the hidden behavior. And thanks for your example of multi-select using angular. It looks really cool! I also thought about using angular, but I really want the UI elements and charts can be in the same paragraph, that's why I created this pull request.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-183723243
@zhongneu me either, I didn't know that such syntax ${label:type=...} exists.
But you did not answer my question. How can I create a dynamic form with checkbox without using ZeppelinContext ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-zeppelin/pull/713
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by zhongneu <gi...@git.apache.org>.
Github user zhongneu commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-189120617
All tasks are done. Ready for final review
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by zhongneu <gi...@git.apache.org>.
Github user zhongneu commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-185586523
I've refactored the variable parsing form a little bit to make the parsing/substation logic consistent, as well as added some unit tests for input parsing.
The checkbox form now can support custom delimiter using syntax like:
`${checkbox( or ):mycheck=a|b,a|b|c}`
This example will be expanded to `a or b`. See the updated screenshots for demo
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by zhongneu <gi...@git.apache.org>.
Github user zhongneu commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-183729056
@doanduyhai Sorry that my last reply is a little bit obscure. I mean we can use syntax like this: `${mycheck:checkbox=default1|default2,value1|value2|value3}`
The query substitution implementation is still in the TODO list. I just want to make sure it won't break anything by using the type syntax here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by zhongneu <gi...@git.apache.org>.
Github user zhongneu commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-184118084
I was a little bit worried about this approach may not be very robust: we have several reserved characters: `${}|(),`. Because the users will put more syntactic elements inside, they may conflict with the reserved characters. However, this is definitely a promising solution, if we document it appropriately.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-189680443
Merge if there're no more discussions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by corneadoug <gi...@git.apache.org>.
Github user corneadoug commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-183999563
You can already add a lot of different dynamic forms to one single paragraph and it won't look good.
Hiding better be at the form level (dynamic forms container), than for one specific element.
Also it depends what you want to achieve with a checkbox, originally it is the kind of component you use to activate an option, not to make multiple selections.
If the goal is to have a multi-select, we better work on a multi-select dropdown.
(I already shared an example using angular display: https://www.zeppelinhub.com/viewer/notebooks/aHR0cHM6Ly9yYXcuZ2l0aHVidXNlcmNvbnRlbnQuY29tL2Nvcm5lYWRvdWcvWmVwcGVsaW4tTm90ZWJvb2tzL21hc3Rlci9BdXRvLUNvbXBsZXRlLU11bHRpU2VsZWN0L25vdGUuanNvbg)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-184112715
Right, multi-selection using string substitution can be complex, considering target language syntax.
How about just concat selections and let user take care target language syntax?
Let's say syntax is
`${mycheck:checkbox=default1|default2,value1(label1)|value2(label2)|value3(label3)}`
And if target language is SQL, filtering can be written like
```
select * from table where 1=1 ${color:checkbox=or color=yellow,or color=yellow(yellow)|or color=red(red)}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by corneadoug <gi...@git.apache.org>.
Github user corneadoug commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-183944263
I'm not sure show/hide option is really necessary in general, since the goal of the dynamic form is to interact with it. (We don't even hide it in report mode)
However if we want that type of behavior, it would be better to have it implemented in a more general way for all dynamic forms, and therefore in a different PR
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-183729488
Ok great, thanks for the answer
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by AhyoungRyu <gi...@git.apache.org>.
Github user AhyoungRyu commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-183611676
Hi @zhongneu, thanks for your contribution!
For the quick review of this PR, could you apply our [PR template](https://github.com/apache/incubator-zeppelin/blob/master/CONTRIBUTING.md#creating-a-pull-request) to your description? : )
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by corneadoug <gi...@git.apache.org>.
Github user corneadoug commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-187293536
So, from what I saw in the front-end code, a checkbox is considered as a group of checkbox by default?
I'm going look into it a bit more, in the mean time @zhongneu could you take care of the last point of your todo list (documentation), so that we can proceed to merge once its reviewed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by AhyoungRyu <gi...@git.apache.org>.
Github user AhyoungRyu commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-183612783
@zhongneu Really appreciate for your quick response! It's not a mandatory, but it will be good if you create a new one : )
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-189377126
z.checkbox() works well in Spark and PySpark. also string substitution works well.
Thanks @zhongneu for great work.
Looks good to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by doanduyhai <gi...@git.apache.org>.
Github user doanduyhai commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-183671166
@zhongneu What is the syntax for this dynamic checkbox ?
For example, for input text, the syntax is `${input_text_label=default value}`
For dynamic drop down menu, it is `${drop_down_label=default value|value 1|value 2| ....|value N}`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by zhongneu <gi...@git.apache.org>.
Github user zhongneu commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-184021909
I found that it is difficult to support multi-selection using string substitution: ${var=value}, because we do not know how to expand it properly in a query. We cannot simply expand it into comma-separated string, because the users may want to use them in several other places (such as filtering conditions). This will make the substitution syntax more complex.
Could we support checkboxes in ZeppelinContext exclusively?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by zhongneu <gi...@git.apache.org>.
Github user zhongneu commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-183993992
I think checkbox is a little bit different from other types of forms, because we may have many checkboxes in a form, and my not look nice if we cannot hide them.
How about provide an option to the users to control the default value of hidden for checkboxes?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-189785882
looks good - great contribution!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by zhongneu <gi...@git.apache.org>.
Github user zhongneu commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-183612251
@AhyoungRyu updated! Shall I create a JIRA for this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-185889236
I tried and it works really well :-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: Add checkbox as a type of dynamic...
Posted by zhongneu <gi...@git.apache.org>.
Github user zhongneu commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/713#issuecomment-188623423
@corneadoug The naming of the css class is a little bit confusing. I cannot use "checkbox" directly, because it seems it will inherit some styles from other classes also named "checkbox". Shall I use a better name, such as "checkbox-input" or is there anyway to prevent it inheriting from other checkbox classes?
I've updated the documentation. Added another task to support pyspark interpreter.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---