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

Close html tag immediatelly - add nested Form support #1275

Merged
merged 19 commits into from
Jul 14, 2020

Conversation

mvorisek
Copy link
Member

@mvorisek mvorisek commented Jun 13, 2020

Nesting html <form> tags is not possible, hovewer they can be closed immediatelly and linked with the input controls/tags, see https://stackoverflow.com/questions/3430214/form-inside-a-form-is-that-alright#21900324

Atk should support AST as much as possible, this is also how some React components cope with this html limitation.

forms spec: https://www.w3.org/TR/html52/sec-forms.html

src/App.php Outdated Show resolved Hide resolved
src/Form.php Outdated Show resolved Hide resolved
src/Form.php Show resolved Hide resolved
@mvorisek
Copy link
Member Author

mvorisek commented Jun 22, 2020

@ibelar, @georgehristov, can you please help with this?

What I have done:

  • replaced the owner <form> tag with div

  • added hidden/immediatelly closed

    tag and linked the controls to it (with form attribute)

  • now load /demos/form/form.php and compare the rendered source from this PR and develop -> this seems to be good

  • I tried to replace the JS selectors by setting the 3rd param to be $this->formElement

but it seems to not work, the the content is not reloaded properly (compare vs. expected result on the demo above on develop)

it seems like some atk issue. JQuery serialization seems to be working well with the linked (ie. not in subtree of <form>) controls...

@ibelar
Copy link
Contributor

ibelar commented Jun 23, 2020

@mvorisek - I have taken a quick look at this.

First: The HTML element set to fire the Fomantic api request is usually set to the form id. So form submit correctly but it is replacing content of the form, which in this PR has a display: none css property.

You would need to adjust this line Form::class:

 $this->js(true)
            ->api(array_merge(['url' => $cb->getJSURL(), 'method' => 'POST', 'serializeForm' => true], $this->apiConfig))
            ->form(array_merge(['inline' => true, 'on' => 'blur'], $this->formConfig));

Probably replacing with the id selector where you want to replace content.

new jQuery('#my_id')->js(true)->api()...

The other problem and I think this would be a major one, is the way Fomantic-UI is handling its form module. Especially when you need to display an error on a specific field. They have a namespace/selector mapping that you can set, but I never try and do not know if it will work. See https://fomantic-ui.com/behaviors/form.html#dom-settings

I am afraid that even if you manage to adjust field selector, their search for input is limited to the enclosed form tag, i.e. search is done inside form, not outside.

I have test manually in browser console and Fomantic throws error:

$('#atk_layout_maestro_tabs_tabssubview_form_2').form('add prompt', 'name', 'error');

@mvorisek
Copy link
Member Author

mvorisek commented Jun 24, 2020

$this->js(true)
        ->api(array_merge(['url' => $cb->getJSURL(), 'method' => 'POST', 'serializeForm' => true], $this->apiConfig))
        ->form(array_merge(['inline' => true, 'on' => 'blur'], $this->formConfig));

This was the issue - this means two things that were written together (not horibly bad, but I did not realized it, thanks)

The other problem and I think this would be a major one, is the way Fomantic-UI is handling its form module. Especially when you need to display an error on a specific field. They have a namespace/selector mapping that you can set, but I never try and do not know if it will work. See https://fomantic-ui.com/behaviors/form.html#dom-settings

I am afraid that even if you manage to adjust field selector, their search for input is limited to the enclosed form tag, i.e. search is done inside form, not outside.

It seems .form() on owning div is working great. It seems they already count with input controls not wrapped in <form>.

So current situation is:

@ibelar wdyt?

@mvorisek mvorisek force-pushed the close_form_immediatelly branch 4 times, most recently from d4fa73e to 4d58f5d Compare June 24, 2020 13:57
@mvorisek mvorisek changed the title WIP Close html tag immediatelly - add nested Form support Close html tag immediatelly - add nested Form support Jun 24, 2020
src/Form.php Outdated Show resolved Hide resolved
@mvorisek mvorisek force-pushed the close_form_immediatelly branch 2 times, most recently from 949da9f to 51c7833 Compare July 8, 2020 23:27
src/Form.php Outdated Show resolved Hide resolved
@mvorisek mvorisek marked this pull request as ready for review July 14, 2020 07:06
@mvorisek mvorisek added the RTM label Jul 14, 2020
@mvorisek mvorisek requested a review from ibelar July 14, 2020 07:21
Copy link
Contributor

@ibelar ibelar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants