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

[EuiFormRow] Add isDisabled prop #4899

Closed
j-m opened this issue Jun 18, 2021 · 4 comments · Fixed by #4908
Closed

[EuiFormRow] Add isDisabled prop #4899

j-m opened this issue Jun 18, 2021 · 4 comments · Fixed by #4908

Comments

@j-m
Copy link
Contributor

j-m commented Jun 18, 2021

This maybe a little outside your intended use-case or design, but I use EuiFormRow like so:

<EuiFormRow label="Some description:">
  <>
   {status=== NOT_STARTED && (
     <EuiText>
        You don't have permission to...
      </EuiText>
   )}

    {status=== IN_PROGRESS && (
      <EuiLoadingContent
        role="progressbar"
        aria-label="loading something"
        lines={1}
      />
    )}

    {status === ERROR && (
      <>
        <SomeInlineErrorComponent error={someError} />
        <EuiButton onClick={() => retry()}>Retry</EuiButton>
      </>
    )}

    {status === SUCCESS && (
      <EuiSelect
        ...
      />
    )}
  </>
</EuiFormRow>

Currently, the label is clickable in all circumstances. However, the label should only be clickable when EuiSelect is rendered, i.e. when there's an input to actually focus on. As EUI cannot easily determine if the child is selectable, it'd make sense to add the prop isLabelDisabled. If the label is disabled then it'd just render a normal EuiFormLabel, with no for attribute.

I have swapped to using EuiFormLabel a rendering a EuiFormRow only for the success state, but then I end up with poor spacing so have to also conditionally render an EuiSpacer.

@cchaos
Copy link
Contributor

cchaos commented Jun 21, 2021

Yeah, there's a lot of problems with the way EuiFormRow works right now without any real connection between it and the form input. The idea is to update the form input control components themselves to accept label, errors and other form row options directly. Here's an example PR the basic text input. #3837

In the short term, I'd say that EuiFormRow should just accept a general isDisabled prop and disable all the necessary bits.

@cchaos cchaos changed the title [EuiFormRow] Add isLabelDisabled prop [EuiFormRow] Add isDisabled prop Jun 21, 2021
@babs20
Copy link
Contributor

babs20 commented Jun 21, 2021

I would be happy to work on this issue. I just want to make sure I understand the intended behavior for the prop before I dive in. Basically, when the prop is true do we not render the input field entirely or just disable it from being focused?

@myasonik
Copy link
Contributor

We still want to render the input, but add the disabled prop to those child <input /> elements

@j-m
Copy link
Contributor Author

j-m commented Jun 23, 2021

To elaborate the issue, here is a codepen of the above code with each variation.
(The "dont have permission to fetch" variation made no sense as I wouldn't've displayed the row at all, but I realised I forgot to add the "don't have permission to edit" variation).

I love the UX of labels focusing their inputs, but it doesn't make sense to any scenario other than enabled.

The existing PR was correct in that disabled form row's should disable their inputs (this is not my goal but is definitely relevant and is not a breaking change as disabled doesnt currently exist for form row) but it should also make the label appear like regular text.

And just to clarify & backtrack on my statement in the issue

If the label is disabled then it'd just render a normal EuiFormLabel, with no for attribute

The for attribute should still be present, but we should simply change the appearance to mimic a label without a for i.e., set cursor: default

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

Successfully merging a pull request may close this issue.

4 participants