Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(Input): inline text - adding inline prop for Input component #120

Merged
merged 10 commits into from
Sep 11, 2018

Conversation

alinais
Copy link
Contributor

@alinais alinais commented Aug 21, 2018

Input Inline Text

Adding inline property to Input component to make possible the inline text addition.

TODO

  • Conformance test
  • Minimal doc site example
  • Stardust base theme
  • Teams Light theme
  • Teams Dark theme
  • Teams Contrast theme
  • Confirm RTL usage
  • W3 accessibility check
  • Stardust accessibility check
  • Update glossary props table
  • Update the CHANGELOG.md

API Proposal

inline

image

The input can be added inline with the text.

(
  <div>Some text inline with the <Input inline placeholder="input name" /> and more text.</div>
)
<div>
  Some text inline with the
  <div class="ui-input ..." placeholder="input name" type="text" value="">
    <input type="text" placeholder="input name" value="" class="..." />
  </div> and more text.
</div>

@codecov
Copy link

codecov bot commented Aug 21, 2018

Codecov Report

Merging #120 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #120   +/-   ##
=======================================
  Coverage   91.15%   91.15%           
=======================================
  Files          59       59           
  Lines        1018     1018           
  Branches      159      140   -19     
=======================================
  Hits          928      928           
  Misses         86       86           
  Partials        4        4
Impacted Files Coverage Δ
src/components/Input/Input.tsx 85.18% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b851b3d...dbd6e67. Read the comment docs.

@alinais alinais changed the title feat(Inut): adding label and labelPosition props for Input component feat(Input): inline text - adding label and labelPosition props for Input component Aug 21, 2018
@kuzhelov kuzhelov added the question Further information is requested, concerns that require additional thought are raised label Aug 24, 2018
@kuzhelov
Copy link
Contributor

kuzhelov commented Aug 24, 2018

do we see these prioperties being implemented as part of the Input component? Or those should be implemented as part of higher-level one that is form-related (FormGroup)?

Also, with this approach there is another question that arises: why we assume that labels will be placed horizontally in regards to input field? What will be labelPosition value for the following case:

image

I assume that a way to achieve this look is to use something like the following:

<Input ... label=' ..' labelPosition='start' styles={{ label: () => { display: 'block' } }}

This approach seems to be inconsistent with those that are about positioning label horizontally.

@kuzhelov kuzhelov added help wanted Extra attention is needed and removed 🚀 ready for review help wanted Extra attention is needed labels Aug 24, 2018
@levithomason
Copy link
Member

Agree, this should be part of the Form abstraction. I would propose a <Form.Field /> consist of a label and a control. This then allows the Form, Form.Field and Form.Group to do support all sorts of layouts.

If we implement this on a component by component basis, then we'll need to duplicate it for every form control component (TextArea, Dropdown, etc).

@levithomason
Copy link
Member

Rethinking this here, does the label use case in the original post fall into this kind of category?

image

I can sort of see that, especially with a basic label but without the borders:
image

<Input
  label={{ content: 'Your name', basic: true }}
  placeholder='My name'
/>

The label in the original post still feels more like form concern to me, but figured I'd post this here to point out that SUIR Input's do support a label. However, it is about "attaching a label to the input" itself opposed to adding a label as in a form.

@kuzhelov
Copy link
Contributor

kuzhelov commented Aug 24, 2018

TL;DR

in that case we should pay serious attention to things that are provided as label prop of Input - if this prop will be abused, the result will break accessibility rules. The problem is that purpose of label prop is very easy to misinterpret by user, given that label elements are usually associated with inputs and have drastically different semantics.


@levithomason, got your point on that - however, lets consider this particular case:

image

Here the problem that I might see relates to accessibility. What will the HTML rendered from this Input? Due to we have the content that serves as form's label (speaking of its semantics - 'Your name'), we should expect something like this:

<div ...>
  <label ... >Your name</label>
  <input type='text' placeholder='...' />
</div>

However, instead of label in that case div will be rendered - and this will break accessibility.


@levithomason
Copy link
Member

levithomason commented Aug 29, 2018

It is worth noting that the SUIR Label in an Input is not intended as the same HTML label, such as in a Form field. It is actually the combination of the Label component with the Input component, which usually has no relation to describing the input itself.

Examples, each of these Inputs has a Label component on one side or the other. You can easily imagine a proper form field label above each one, which would be a separate concern:

image

image

image

image

image

image

image

For accessibility purposes none of these label "attachments" (if I can call it that) seem to be the "form field label" to me. I can image a proper HTML label above each of these which properly describes the field itself.

@alinais alinais changed the title feat(Input): inline text - adding label and labelPosition props for Input component feat(Input): inline text - adding inline prop for Input component Sep 7, 2018
@alinais
Copy link
Contributor Author

alinais commented Sep 7, 2018

Removing the approach with label. This will be further discussed and agreed on as part of Form Field implementation.
Updating the PR/ API proposal to use one inline prop for the Input which will basically update only the css in order to ensure that the input will appear as desired. The question remains whether we would like to render only the input or the full Input container.
@kuzhelov @levithomason any thoughts?

@alinais alinais added help wanted Extra attention is needed 🚀 ready for review and removed question Further information is requested, concerns that require additional thought are raised help wanted Extra attention is needed labels Sep 7, 2018
@alinais alinais merged commit fb1a6e8 into master Sep 11, 2018
@levithomason levithomason deleted the feature/input-inline-text branch October 29, 2019 22:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants