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

MF-21: Build Textfields #23

Merged
merged 3 commits into from Aug 20, 2019

Conversation

@fatmali
Copy link
Collaborator

commented Aug 16, 2019

@joeldenning
Copy link
Collaborator

left a comment

Screen reader accessibility problem

See #23 (comment). That comment has potential to cause a rework of the HTML contract for inputs.

Other thing to discuss

One thing that is missing from the styleguide designs is inputs whose labels are to the side instead of on top. For example:

image

Labels such as these are helpful for really long forms, because there are nice vertical lines that the eye can scan easily to find the label text you're looking for.

In my opinion, OpenMRS should have both kinds of inputs and use them depending on if it's a really long form or a short form. I don't think we need to worry designing and building this other kind of input label right now, but I want to make sure that our HTML/CSS contract from the styleguide will be suitable for when we use labels that are not embedded right next to the input.

Do you think the current HTML/CSS contract will be usable for situations like that?

input.omrs-input-filled {
border: none;
border-bottom: 2px solid var(--omrs-color-ink-lowest-contrast);
border-radius: 0.25rem 0.25rem 0px 0px;

This comment has been minimized.

Copy link
@joeldenning

joeldenning Aug 16, 2019

Collaborator

nit: use 0 instead of 0px. The number is the only one where you don't need to provide a unit.

It makes no functional difference, but I'm just encouraging people to use rem whenever possible instead of px.

input.omrs-input-filled,
input.omrs-input-outlined {
font-size: 1rem;
display: block;

This comment has been minimized.

Copy link
@joeldenning

joeldenning Aug 16, 2019

Collaborator

Inputs are display: inline by default. Changing it to display: block will make inputs not stay on the same horizontal line like they normally do.

I would prefer not changing the default display for inputs, so that people used to how inputs behave don't have to learn/adjust to what OpenMRS styleguide is doing.

Are you using block so you can do vertical-align: middle? One way to solve that is to just add padding-top and padding-bottom to vertically center the text within an input.

src/inputs/input.css Show resolved Hide resolved
src/inputs/input.css Show resolved Hide resolved

input.omrs-input-underlined:focus:not(.omrs-input-danger) svg,
input.omrs-input-filled:focus:not(.omrs-input-danger) svg {
fill: black;

This comment has been minimized.

Copy link
@joeldenning

joeldenning Aug 16, 2019

Collaborator

Looking at Figma, I think this maybe should be var(--omrs-color-ink-high-contrast)??


const isDanger = boolean("is Danger", false);

const determineStyle = (inputType, isDanger) => {

This comment has been minimized.

Copy link
@joeldenning

joeldenning Aug 16, 2019

Collaborator

nit: this function could be simplified:

const determineStyle = (inputType, isDanger) => `omrs-input${inputType} ${isDanger ? 'omrs-input-danger' : ''}`.trim()

const determineStyle = (inputType, isDanger) => {
let styles = `omrs-input-${inputType}`;
isDanger ? (styles = `${styles} omrs-input-danger`) : "";

This comment has been minimized.

Copy link
@joeldenning

joeldenning Aug 16, 2019

Collaborator

It is poor practice to modify a variable inside of a ternary -- ternaries are meant for returning a new value.

If you want to modify a value instead returning a new value, use if else. When transitioning to React, it's common for people to think that if statements are somehow no longer okay. But that is not true.

<input class="omrs-input-underlined" type="text" required />
<label>Name</label>
<span class="omrs-input-helper">Helper Text</span>
<svg

This comment has been minimized.

Copy link
@joeldenning

joeldenning Aug 16, 2019

Collaborator

Shouldn't we be using styleguide icons for this? e.g.

<svg class="omrs-icon" role="img">
  <use xlink:href="#omrs-icon-visibility"></use>
</svg>
required
/>
<label>Tel</label>
<svg

This comment has been minimized.

Copy link
@joeldenning

joeldenning Aug 16, 2019

Collaborator

same here about using styleguide icons.

type="text"
required
/>
<label>Tel</label>

This comment has been minimized.

Copy link
@joeldenning

joeldenning Aug 16, 2019

Collaborator

This HTML is inaccessible to screen readers. The screen reader won't know that this <label> applies to this <input>.

See https://kentcdodds.com/blog/please-stop-building-inaccessible-forms-and-how-to-fix-them for options to fix.

This comment has been minimized.

Copy link
@joeldenning

joeldenning Aug 16, 2019

Collaborator

I think I maybe lean towards adding for attributes and id attributes. But am open to the other solutions that Kent suggests in that blog post. What do you think?

This comment has been minimized.

Copy link
@fatmali

fatmali Aug 19, 2019

Author Collaborator

Since the behavior of the label relies very much on the input e.g on focus, it is might be very difficult to achieve the same behavior if we move the label up to be on top of the input without using some JavaScript i.e

<label for="name>Name</label>
<input class="omrs-input-outlined" id="name" />

However, I gave this a shot and it seemed to work alright with the accessibility voiceover :

<input class="omrs-input-outlined" id="name" />
<label for="name>Name</label>

Another option I can think of might be to define the html contract like this

<label class="omrs-input-outlined">
  <input>
  <span>Name</span>
</label>

This comment has been minimized.

Copy link
@joeldenning

joeldenning Aug 20, 2019

Collaborator

Since the behavior of the label relies very much on the input e.g on focus, it is might be very difficult to achieve the same behavior if we move the label up to be on top of the input without using some JavaScript

Could you help me understand this? I am unaware of any differences in accessibility when the <label> is before the <input> vs after the <input>. Is that what you're saying? That a <label> before the <input> doesn't work with screen readers? Or am I misunderstanding?

This comment has been minimized.

Copy link
@joeldenning

joeldenning Aug 20, 2019

Collaborator

That last option you suggested would make it pretty difficult to implement the vertical input form layout that I mentioned in my big comment at the top ^. I think I prefer using either <label for=""> or <label id="hi"> with <input aria-labelledby="hi">

This comment has been minimized.

Copy link
@fatmali

fatmali Aug 20, 2019

Author Collaborator

makes sense. With regards to the big comment at the top about horizontally aligned forms, I think it may be possible to achieve that with the current html css contract using
display: flex and the order property to place the label on the left of the input. I'm thinking we'd have to replace .omrs-input-group with .omrs-input-group-horizontal to achieve that.

This comment has been minimized.

Copy link
@fatmali

fatmali Aug 20, 2019

Author Collaborator

Pushed new changes according to the review @joeldenning

fatmali added 2 commits Aug 20, 2019
@joeldenning
Copy link
Collaborator

left a comment

looks good - one last thing to change.

<div class="stories-example lang-html">
<div class="omrs-input-group">
<input
for="demoInput"

This comment has been minimized.

Copy link
@joeldenning

joeldenning Aug 20, 2019

Collaborator

This should be id="demoInput", right? The <input> should have the id and the <label> should have the for attribute

This comment has been minimized.

Copy link
@fatmali

fatmali Aug 20, 2019

Author Collaborator

sure

@joeldenning

This comment has been minimized.

Copy link
Collaborator

commented Aug 20, 2019

I'm just going to merge and make that small documentation change myself, to speed things up.

@joeldenning joeldenning merged commit a99a150 into openmrs:master Aug 20, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fatmali fatmali deleted the fatmali:MF-21 branch Aug 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.