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

Render id={form.identifier} around ConfirmationFinisher #129

Closed
wants to merge 1 commit into from

Conversation

paavo
Copy link

@paavo paavo commented Jan 5, 2021

Adds a div container with the form identifier to the ConfirmationFinisher so that it is scrolled into view upon invokation

Resolves: #130

The `<form>` is rendered always with`id={form.identifier}`
After submitting the Form `#{form.identifier}` will be added to the URL. 
If the Form is somewhere on the Page, the anchor `#{form.identifier}` will jump to the Form. 

The `Neos.Form.Builder:ConfirmationFinisher` does NOT render `id={form.identifier}` around. 
Because of the missing id, the Browser doesn't jump to the Confirmation Message and it maybe get lost somewhere on the page. 

What about wrapping an `id={form.identifier}` around the Builder:ConfirmationFinisher?
@bwaidelich
Copy link
Member

@paavo Thanks a lot for this. I tweaked the PR description a little and will look into this asap

@bwaidelich
Copy link
Member

I still didn't get around looking deeper into this.
But there is a fundamental issue with that that prevents me from just hitting the "Merge" button:
Finishers should be mostly independent from the Form rendering since they can be used in all kinds of context (e.g. CLI forms).

The change makes a lot of sense for the general case, but it should probably be configurable in some way..

For now I would suggest to use a custom Finisher (in general I think it was a mistake to ship the core form framework with all the element and finisher implementations).

In this case, you should be able to achieve the fixed behavior with sth like

class CustomConfirmationFinisher extends ConfirmationFinisher {
   protected function executeInternal() {
       parent::executeInternal();
       $formRuntime = $this->finisherContext->getFormRuntime();
       $formRuntime->getResponse()->setContent('<div id="' . $formRuntime->getIdentifier() . '">' . $formRuntime->getResponse()->getContent() . '</div>');
   }
}

@paavo
Copy link
Author

paavo commented Jan 21, 2021

Thanks for your Input @bwaidelich

Finishers should be mostly independent from the Form rendering since they can be used in all kinds of context (e.g. CLI forms).

I didn't knew about that, but makes sense.

I just wrapped a Container (with the Form-ID) around the Form (including the ConfirmationFinisher) in Fusion.

@paavo paavo closed this Jan 21, 2021
@bwaidelich
Copy link
Member

Thanks for the update!

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.

Render id={form.identifier} around ConfirmationFinisher
2 participants