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

EuiForm should render to form #2272

Closed
flash1293 opened this issue Aug 29, 2019 · 12 comments · Fixed by #3010
Closed

EuiForm should render to form #2272

flash1293 opened this issue Aug 29, 2019 · 12 comments · Fixed by #3010

Comments

@flash1293
Copy link
Contributor

Currently the EuiForm component renders a div as container element. This is slightly confusing, I would have expected it to render an html <form> element, so onSubmit on EuiForm works:

<EuiForm onSubmit={sendToServer}>
   ...
   <EuiButton fill type="submit">Submit form</EuiButton>
</EuiForm>
@thompsongl
Copy link
Contributor

@snide Looks like you built this to render a div back in the day. Any reason for that? Seems like form is the way to go.

@snide
Copy link
Contributor

snide commented Sep 6, 2019

I think form makes sense and what I should have done in retrospect (I did it mostly in the assumption of people wanting to manage form tags on their own). Like the negative flex issue though it's harder to retroactively make that decision. Doing it will require everyone to change their usage of it and it would be a larger breaking change (for example, people might be using it in one component now, but then having the actual form field in a wrapper component). If we think it's worth the effort to dev happiness to make that change all through Kibana / Cloud / Swiftype...etc then it makes sense.

Otherwise it's just a quirky artifact that is odd, but mostly harmless.

I generally advise to let improper naming quirks live, but that's certainly skewed by not wanting to go through the conversion effort.

One way we could help is just by adding an additional prop that turns it into a form.

@flash1293
Copy link
Contributor Author

Thanks for the explanation @snide - I think you are right and the benefits of this breaking change don't justify the effort.

One suggestion: The examples (e.g. https://elastic.github.io/eui/#/forms/form-layouts ) should wrap
EuiForm in a form to emphasize how it should be used.

@nomadhoc
Copy link

It took me a while to realize that EuiForm is not a real form, which is counter-intuitive. I agree that it should be a real <form> instead of <div>.

@nomadhoc
Copy link

One possible solution is to create another component, e.g. EuiHTMLForm, for people who prefer a real form without having to write wrapper. Then EuiForm can be gradually deprecated or shift to using form.

@anishagg17
Copy link
Contributor

anishagg17 commented Mar 8, 2020

@snide should i start working on

One way we could help is just by adding an additional prop that turns it into a form.

@snide
Copy link
Contributor

snide commented Mar 9, 2020

@anishagg17 Sure. Following convention I'd use a component prop that can take form, but defaults to div. Check EuiFlexGroup for an example.

@j-m
Copy link
Contributor

j-m commented Apr 14, 2021

What is the use case for div? Why is the default not form?
At the very least div should add the ARIA role "form"

See also #4416

@cchaos
Copy link
Contributor

cchaos commented Apr 14, 2021

Yep, we understand, it's a legacy piece of code that would be a major breaking change if we changed the default. You can change it with component="form" or add the aria role.

@j-m
Copy link
Contributor

j-m commented Apr 14, 2021

Not what I wanted to hear but completely understandable, all breaking changes cost something. However, at the same time, this raises new concerns for me; I don't want import features, like accessibility, to be blocked by technical debt/cost from existing adopters. I choose EUI because it's "Accessible to everyone" so, regardless of the default element, I suggest adding the aria role as a minor bump

Just FYI, the use of form is pretty inconsistent throughout the doc demos, e.g. Form Layouts alone has an example that uses component="form", three that don't, one that uses Fragment, and two that use EuiFlexGroup (understandable for inline, but should still have a Form wrapper).

Yeah, I knew I'd fall for it a few times so one of the first things I did when I adopted EUI was create a component wrapper that used EuiFormProps and set component="form". Came across the wrapper again today during a refactor and wondered if it was still needed

@myasonik
Copy link
Contributor

Thanks for feedback! It's really great to hear that folks outside of Elastic are choosing EUI because accessibility is one of it's primary goals.

Unfortunately, we're a small team and can't tackle everything. You can see a whole swath of a11y issues that we're aware of and would like to get to but haven't had the capacity to address.

We're constantly getting better though so keep these issues coming (PRs too, if you can!) and we'll keep moving forward.

@j-m
Copy link
Contributor

j-m commented Apr 14, 2021

Thank you for your candour, and for you and your team's great work!
I continue to be impressed by the EUI team's attitude and passion towards this project, and love how open you are to community feedback.

I know my comments and issues probably come across as gripes but I assure you that I'm not trying to be mean. I love EUI and want to see it get the attention it deserves. Thank you for your patience, everyone ❤

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 a pull request may close this issue.

8 participants