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

Input: new style #459

Merged
merged 8 commits into from Aug 13, 2019

Conversation

@bpierre
Copy link
Member

commented Aug 12, 2019

image

  • Use the new theme colors.
  • Use the new text styles.
  • No shadow anymore.
  • “Selected” (focused) state.

@bpierre bpierre requested review from AquiGorka, sohkai and dizzypaty Aug 12, 2019

@sohkai

sohkai approved these changes Aug 13, 2019

Copy link
Member

left a comment

LGTM!

`}
/>
)
}

TextInput.propTypes = {
required: PropTypes.bool,

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 13, 2019

Member

Perhaps we don't even need the type prop declarations anymore, I think React will complain for us if it tries to set a type that isn't valid?

They especially wouldn't be valid with multiline (which we should add to the prop declarations and to the README!)

This comment has been minimized.

Copy link
@bpierre

bpierre Aug 13, 2019

Author Member

mmm are you thinking of only having it in the documentation? Or maybe using PropType.string, to not limit it to the existing list of types we have. But I consider it to be part of the API otherwise.

They especially wouldn't be valid with multiline

Yes it shouldn’t be passed to textarea. 👍

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 13, 2019

Member

Or maybe using PropType.string, to not limit it to the existing list of types we have

This might make the most sense? Does react error if you try to give an <input> an unknown type (I thought it started doing so)?

This comment has been minimized.

Copy link
@bpierre

bpierre Aug 13, 2019

Author Member

Does react error if you try to give an an unknown type (I thought it started doing so)?

I’m not sure, but I guess it would be the user’s responsibility at that point?

This comment has been minimized.

Copy link
@bpierre

bpierre Aug 13, 2019

Author Member

I added a few commits 👇

bpierre added some commits Aug 13, 2019

@sohkai

sohkai approved these changes Aug 13, 2019

Copy link
Member

left a comment

Some small last comments, but LGTM 🎉

src/components/Input/TextInput.md Outdated Show resolved Hide resolved
src/components/Input/TextInput.md Outdated Show resolved Hide resolved

bpierre and others added some commits Aug 13, 2019

Update src/components/Input/TextInput.md
Co-Authored-By: Brett Sun <qisheng.brett.sun@gmail.com>

@bpierre bpierre merged commit 6c22a44 into newstyle Aug 13, 2019

1 of 4 checks passed

continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
License Compliance FOSSA is analyzing this commit
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
WIP Ready for review
Details

@delete-merged-branch delete-merged-branch bot deleted the newstyle-input branch Aug 13, 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.