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

feat(text-field): add placeholder property #930

Merged

Conversation

rbare1
Copy link
Contributor

@rbare1 rbare1 commented Jun 13, 2019

related to #224

Reopening of #840, since that was targeting an old feature branch.

@codecov-io
Copy link

codecov-io commented Jun 13, 2019

Codecov Report

Merging #930 into rc0.14.0 will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           rc0.14.0     #930      +/-   ##
============================================
+ Coverage     94.08%   94.09%   +<.01%     
============================================
  Files            86       86              
  Lines          3637     3640       +3     
  Branches        573      575       +2     
============================================
+ Hits           3422     3425       +3     
  Misses           90       90              
  Partials        125      125
Impacted Files Coverage Δ
packages/text-field/index.tsx 98.32% <100%> (+0.05%) ⬆️
packages/text-field/Input.tsx 100% <0%> (ø) ⬆️

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 cc06add...99a1445. Read the comment docs.

@moog16
Copy link

moog16 commented Jun 13, 2019

Can you change the branch to rc0.14.0

@rbare1 rbare1 force-pushed the feat/text-field-placeholder-text branch 2 times, most recently from ba1a08f to 26e7859 Compare June 14, 2019 08:56
@rbare1 rbare1 changed the base branch from master to rc0.14.0 June 14, 2019 08:59
@rbare1 rbare1 force-pushed the feat/text-field-placeholder-text branch from 26e7859 to 0a69c32 Compare June 14, 2019 10:16
Copy link

@moog16 moog16 left a comment

Choose a reason for hiding this comment

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

Looks like there is an issue with icons. Can you please check this out?
j71yQVCvOum

@moog16
Copy link

moog16 commented Jun 14, 2019

Please ping me when this is ready for rereview.

@rbare1
Copy link
Contributor Author

rbare1 commented Jun 17, 2019

Looks like there is an issue with icons. Can you please check this out?
j71yQVCvOum

@moog16 This is an issue in MDC Web that has been fixed in v2.1.0
material-components/material-components-web#4637
https://github.com/material-components/material-components-web/blob/master/CHANGELOG.md#210-2019-05-06

@moog16
Copy link

moog16 commented Jun 17, 2019

@rbare1 thanks for linking those together. Could you update text field to latest v2.3.1? There are no breaking changes.

refs #885

@rbare1
Copy link
Contributor Author

rbare1 commented Jun 18, 2019

@moog16 Updated!

Copy link

@moog16 moog16 left a comment

Choose a reason for hiding this comment

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

Please resolve conflicts. Looks good!

@rbare1 rbare1 force-pushed the feat/text-field-placeholder-text branch from 1247795 to 839b6fe Compare June 19, 2019 08:57
@rbare1
Copy link
Contributor Author

rbare1 commented Jun 19, 2019

@moog16 Rebased onto rc0.14.0

@moog16
Copy link

moog16 commented Jun 19, 2019

Can you add to the test/screenshot/golden.json file:

"text-field/noLabel": "03812f8c2eeb6bbfeb5bae4cdc7794a7ca760628e119edaec974d8f87740453c"

@rbare1
Copy link
Contributor Author

rbare1 commented Jun 20, 2019

The golden for fullWidth needs updated as well, since it has no label and gets the same mdc-text-field-no-label class that had the padding change in MDC Web 2.1.0

@moog16 moog16 merged commit 0818061 into material-components:rc0.14.0 Jun 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants