Skip to content

[TextField] Fixed support for the componentId attribute in component TextField #3897

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.

Sign up for GitHub

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

Jump to bottom
Closed
IASamoylov wants to merge 4 commits into from
Closed

[TextField] Fixed support for the componentId attribute in component TextField #3897

IASamoylov wants to merge 4 commits into from

Conversation

@IASamoylov
Copy link
Contributor

@IASamoylov IASamoylov commented Feb 6, 2018 •
edited
Loading

Pull request checklist

Description of changes

  • changed initialization attributed componentId, added default value this._id
  • changed initialization of the variable renderProps
  • changed render function for and
  • added test to check attribute

Ivan Samoylov added 2 commits February 7, 2018 01:49
fixed support for the componentId attribute
a5afef9 
+ changed initialization attributed componentId, added default value this._id
+ changed initialization of the variable renderProps
+ changed render function for <input/> and <area/>
+ added test to check attribute
Include a change request file
521bf2c
@msftclas
Copy link

msftclas commented Feb 6, 2018 •
edited
Loading

CLA assistant check
All CLA requirements met.

@IASamoylov IASamoylov mentioned this pull request Feb 7, 2018
Merged
2 tasks
@IASamoylov IASamoylov changed the title Fixed support for the componentId attribute in component TextField [TextField] Fixed support for the componentId attribute in component TextField Feb 8, 2018
@dzearing
Copy link
Member

dzearing commented Feb 9, 2018 •
edited
Loading

@micahgodbolt It looks like you added componentId months ago, but never actually put an id anywhere, so the label for attribute points to a missing component. This PR fixes it, which is good, but I also am not sure why we didn't do this:

  1. Use id for the input id. If none is provided, default to a generated one.
  2. Pass that in to the label's htmlFor.

Why would a consumer pass in id vs componentId? It isn't predictable and the documentation is unclear.

dzearing
dzearing requested changes Feb 9, 2018
View reviewed changes
Copy link
Member

@dzearing dzearing left a comment

Choose a reason for hiding this comment

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

I'm confused why we aren't using id instead of componentId. Added question to @micahgodbolt

@IASamoylov
Copy link
Contributor Author

IASamoylov commented Feb 9, 2018

@dzearing, @micahgodbolt if we set TextField's "id" property manually, Label's property "for" value inside the component will not be modifed and still have default one

@IASamoylov
Copy link
Contributor Author

IASamoylov commented Feb 9, 2018 •
edited
Loading

  1. Use id for the input id. If none is provided, default to a generated one.
  2. Pass that in to the label's htmlFor.

@dzearing, I fixed this issues

FIx TextField's id
54fe8b7 
+ Used TextField's id for the input id. If none is provided, default to a generated one.
+ For a custom rendering function creates a new props with a new id (this._id)
@dzearing
Copy link
Member

dzearing commented Mar 27, 2018

I believe with the merge of #3896 we can close this one.

@dzearing dzearing closed this Mar 27, 2018
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dzearing dzearing dzearing requested changes

Assignees

@dzearing dzearing

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[TextField] Custom value for attribute componentId don't work in TextField

3 participants

@IASamoylov @msftclas @dzearing