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

Incorrect helperText space insertet between all components #4425

Closed
sigmunau opened this issue Feb 14, 2020 · 6 comments
Closed

Incorrect helperText space insertet between all components #4425

sigmunau opened this issue Feb 14, 2020 · 6 comments

Comments

@sigmunau
Copy link

sigmunau commented Feb 14, 2020

Since about commit 07ae952 react admin has started putting extra space between most controls, unless helperText={false} is explicitly passed to the component.

What you were expecting:

Inputs are laied out nicely without extra space in between

What happened instead:

Extra space is inserted between all inputs unless helperText is explicitly set to false

Steps to reproduce:

upgrade ra-ui-materialui to version 3.2.2

Related code:

                <NumberInput source="s1" helperText={false} /> <!-- looks nice -->
                <NumberInput source="s2" /> <!-- extra space in layout-->

The rendered html looks like this:

<div class="ra-input ra-input-identityTokenLifetime"><div class="MuiFormControl-root MuiTextField-root RaFormInput-input-264 MuiFormControl-marginDense"><label class="MuiFormLabel-root MuiInputLabel-root MuiInputLabel-formControl MuiInputLabel-animated MuiInputLabel-shrink MuiInputLabel-marginDense MuiInputLabel-filled MuiFormLabel-filled" data-shrink="true" for="identityTokenLifetime" id="identityTokenLifetime-label"><span>Identity token lifetime</span></label><div class="MuiInputBase-root MuiFilledInput-root MuiFilledInput-underline MuiInputBase-formControl MuiInputBase-marginDense MuiFilledInput-marginDense"><input aria-invalid="false" aria-describedby="identityTokenLifetime-helper-text" id="identityTokenLifetime" name="identityTokenLifetime" type="number" step="any" class="MuiInputBase-input MuiFilledInput-input MuiInputBase-inputMarginDense MuiFilledInput-inputMarginDense" value="300"></div><p class="MuiFormHelperText-root MuiFormHelperText-contained MuiFormHelperText-filled MuiFormHelperText-marginDense" id="identityTokenLifetime-helper-text"><span>​</span></p></div></div>

Notice the <p> tag with MuiFormHelperText-root class

Other information:

Environment

  • React-admin version: 3.2.2
  • Last version that did not exhibit the issue (if applicable): Not clear 3.1.3 was ok
  • React version:
  • Browser: firefox
  • Stack trace (in case of a JS error):
@fzaninotto
Copy link
Member

Hi, and thanks for your report.

It's not a bug, it's a feature™!

See rationale and discussion in #4364.

@sigmunau
Copy link
Author

I see now that it is indeed a feature and even mentioned in the relevant release notes. However the fix is really optimistic and it only works at all if the help text will fit on a single line. Given the very narrow default width of controls this is not at all given. So basically you are sacrificing a lot of valuable screen real estate in case some field should give a single-line validation error. A proper fix for the submit button moving away would need to ensure that the validation texts all fit on a single line (which I think would be a UX nightmare).

In any case, when validation fails the user will have to go back and change things, so pressing the save button again without moving the mouse point isn't really a use case. The only real risk I see of having the button move would be if there were some other button with some other behavior (say a delete button) directly above. Then users could theoretically end up typing a invalid value in a field, pressing save and then accidentally pressing delete immediately afterwards

@sigmunau
Copy link
Author

image

screenshot of my applications now very tall layout with the save button still moved out of the way by a not too unreasonable validation error

@fzaninotto
Copy link
Member

The problem is that popping validation messages make the submit button move down, under the mouse cursor, and that prevents the click event (in fact, the sumbit button only received a mouseDown event, and never a mouseup). The previous layout prevented us from properly implementing the save on enter feature, and had other corner cases.

So we basically had to choose between two evils, and we chose to have working features and a less compact ui by default - letting you opting in to a compact UI with helperText={false}.

The heart of the problem, IMHO, is Material Design itself. I can understand that error messages should appear under an input on Mobile, but on Desktop I really think the horizontal layout (where labels and error messages appear aside the input) is better.

@sigmunau
Copy link
Author

As I was trying to point out, validation messages still makes the save button move away. But at least the button responds enough to trigger the validation. I don't see exactly what the flow of event could have been that you were catering for that caused validation to trigger on mouse down but still wanted something to happen on mouse up, but it doesn't much matter. I just want to hint that if you have a real problem with the save button moving away you should probably also handle multi-line validation errors

@fzaninotto
Copy link
Member

@sigmunau I understand your point of view. We can't know in advance if the error message will be multi line or not. We chose to reserve some space for a single line error message.

Again, this is a trade-off. If this trade-off doesn't fit your requirements, you can still use your own input components instead of react-admin's.

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