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

Get rid of setup_form in Ot_popup #112

Open
sagotch opened this issue Dec 16, 2016 · 4 comments
Open

Get rid of setup_form in Ot_popup #112

sagotch opened this issue Dec 16, 2016 · 4 comments

Comments

@sagotch
Copy link
Contributor

sagotch commented Dec 16, 2016

I think it should be moved to another module (probably Ot_form) since every popup is not a form.

You ship more code than necessary if you are not using this feature, and once again, this module is called Ot_popup and not Ot_popup_containing_form.

@dannywillems
Copy link
Member

I'm not against a module called Ot_form but I think popup containing forms must stay in Ot_popup.

@sagotch
Copy link
Contributor Author

sagotch commented Dec 19, 2016

But why?

@dannywillems
Copy link
Member

Because it's a popup. I agree it's a popup containing a form but first it is a popup. Or maybe use Ot_form_popup.

@sagotch
Copy link
Contributor Author

sagotch commented Dec 20, 2016

The point of a popup is to... pop up.
Also, looping through a form inputs and having first field auto focused could be useful for every page or part of a page, not for popup only.

I think there is no need for having a popup specialization for forms. Just put an Ot_form in you Ot_popup and everything will be fine.

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

No branches or pull requests

2 participants