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

Add TextInput (and TextInput.Number) #67

Merged
merged 4 commits into from
Dec 19, 2017
Merged

Add TextInput (and TextInput.Number) #67

merged 4 commits into from
Dec 19, 2017

Conversation

sohkai
Copy link
Contributor

@sohkai sohkai commented Dec 12, 2017

Adds a basic, styled <input /> box.

I've added the README as src/Form/Input.md, since I can see src/Form containing multiple components in the future (all the other input types, etc). In general, it might be interesting to have folders under src/ be groups holding actual components, with each component's documentation living as <component name>.md.

Fixes #49.

@sohkai sohkai requested a review from bpierre December 12, 2017 01:06
}
&:read-only {
color: transparent;
text-shadow: 0 0 0 ${theme.textSecondary};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly for Firefox; Firefox does this weird thing where readonly inputs still show a flashing cursor.

An interesting fix for this is to make the text color transparent, and then use a text-shadow to display text given through value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix :-)

Do you if this is interpreted correctly by screen readers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question, a quick investigation doesn't bring up anything too conclusive but a test using voice over on OSX does read the read only value.

@sohkai sohkai mentioned this pull request Dec 12, 2017
@bpierre
Copy link
Contributor

bpierre commented Dec 13, 2017

Shall we name this component TextInput? The HTML name is super generic, but we probably want something a bit more specific with a limited set of allowed type values.

For example, I think our implementation of <input type=checkbox /> should be CheckBox, <input type=range /> should be <Slider />, etc. What do you think?

I can see src/Form containing multiple components in the future (all the other input types, etc). In general, it might be interesting to have folders under src/ be groups holding actual components

I agree, having components groups will make things clearer, great idea! About Form/ specifically, what would be the criteria to put a component in Form/? In a classic HTML + HTTP app, some elements like <input/> have to be placed in a <form> so they can be submitted, but it’s not true for Web apps, where we can have these components inside or outside a form. For example, a Button can be used anywhere, a CheckBox could be used to filter a list, etc.

I checked what others are doing: iOS uses the name “Controls” to group some of its interactive components, and Android uses the name “Input Controls”. What would you think of using a similar name for this group, instead of Form?

@sohkai
Copy link
Contributor Author

sohkai commented Dec 16, 2017

Shall we name this component TextInput?

👍. I'll make it so TextInput defaults to type="text" and add TextInput.Number for type="number".

What would you think of using a similar name for this group, instead of Form?

Not sure I like Controls/ since it's pretty ambiguous, but how does Field/ or Input/ (or UserInput/) sound? Originally I used /Form because I thought we were going to need a form element, but we can scrap that now.

@bpierre
Copy link
Contributor

bpierre commented Dec 16, 2017

I'll make it so TextInput defaults to type="text" and add TextInput.Number for type="number".

👍

Not sure I like Controls/ since it's pretty ambiguous, but how does Field/ or Input/ (or UserInput/) sound? Originally I used /Form because I thought we were going to need a form element, but we can scrap that now.

Field/ sounds form related, UserInput/ sounds the most “correct”, but Input/ is shorter. So Input/ is my favorite :-)

@sohkai sohkai changed the title Add Input (Text and Number) Add TextInput (and Number) Dec 17, 2017
@sohkai sohkai changed the title Add TextInput (and Number) Add TextInput (and TextInput.Number) Dec 17, 2017
Copy link
Contributor

@bpierre bpierre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@sohkai sohkai merged commit 5efb2f8 into master Dec 19, 2017
@sohkai sohkai deleted the add-input branch February 4, 2018 17:08
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 this pull request may close these issues.

2 participants