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

Conversation

Projects
None yet
2 participants
@sohkai
Copy link
Member

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 Dec 12, 2017

}
&:read-only {
color: transparent;
text-shadow: 0 0 0 ${theme.textSecondary};

This comment has been minimized.

Copy link
@sohkai

sohkai Dec 12, 2017

Author Member

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.

This comment has been minimized.

Copy link
@bpierre

bpierre Dec 13, 2017

Member

Nice fix :-)

Do you if this is interpreted correctly by screen readers?

This comment has been minimized.

Copy link
@sohkai

sohkai Dec 16, 2017

Author Member

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 referenced this pull request Dec 12, 2017

Closed

Add Form #58

@bpierre

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

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

@bpierre
Copy link
Member

left a comment

💯

@sohkai sohkai merged commit 5efb2f8 into master Dec 19, 2017

@sohkai sohkai deleted the add-input branch Feb 4, 2018

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