Skip to content

Adding support to add custom attributes on SpinButton and ComboBox #9872

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

Merged
merged 5 commits into from
Jul 22, 2019
Merged

Adding support to add custom attributes on SpinButton and ComboBox #9872

merged 5 commits into from
Jul 22, 2019

Conversation

pagaur
Copy link
Contributor

@pagaur pagaur commented Jul 19, 2019

Pull request checklist

Description of changes

This change allows some custom attribute to set on Spin button or ComboBox components (BaseButton and Checkbox are already passing custom attributes if passed to it).
The container where we want to host buttons expects a custom property 'data-nav:true' to be set to enable keyboard navigation between various buttons on that surface.

Focus areas to test

These changes should not affect existing

Microsoft Reviewers: Open in CodeFlow

@msftclas
Copy link

msftclas commented Jul 19, 2019

CLA assistant check
All CLA requirements met.

@joschect
Copy link
Contributor

@pagaur I'm surprised that you can't already pass those down through to combobox's autofill and buttonicon. I wonder if there is a typing issue somewhere since you can pass those options directly to autofill. @JasonGore do you have any idea why you might be able to pass a prop directly to a react element but not through a props object?

image
vs

image

@size-auditor
Copy link

size-auditor bot commented Jul 19, 2019

Bundle test Size (minified) Diff from master
SpinButton 177.514 kB ExceedsBaseline     50 bytes
ComboBox 222.667 kB ExceedsBaseline     25 bytes

ExceedsTolerance  Exceeds Tolerance     ExceedsBaseline  Exceeds Baseline     BelowBaseline  Below Baseline     1 kB = 1000 bytes

@JasonGore
Copy link
Member

JasonGore commented Jul 19, 2019

Yes, I just had a discussion with the TypeScript team about this recently. The TypeScript compiler is basically hardcoded to support data- and aria- props passed to elements. Unfortunately this means there's really no easy way to allow them in arbitrary props objects without just allowing all strings. The ability to support that is covered by this TypeScript PR and this TypeScript PR.

For now, I think the only thing you can do is cast data- attributes to any when assigning them to props object props, or create the props object separately and cast it as IAutofillProps.

@msft-github-bot
Copy link
Contributor

msft-github-bot commented Jul 19, 2019

Component Perf Analysis:

Scenario Master Samples * PR Samples *
BaseButton 854 851
BaseButton (experiments) 1727 1719
DefaultButton 1161 1166
DefaultButton (experiments) 1973 1950
DetailsRow 5811 5732
DetailsRow without styles 5499 5424
DocumentCardTitle with truncation 43197 44134
MenuButton 2030 2069
MenuButton (experiments) 4926 4974
PrimaryButton 1399 1409
PrimaryButton (experiments) 2484 2449
SplitButton 3975 3975
SplitButton (experiments) 9005 9116
Stack 514 515
Stack with Intrinsic children 2012 1927
Stack with Text children 5400 5331
Text 415 419
Toggle 997 976
Toggle (experiments) 2439 2460
button 69 75
* Sample counts can vary by up to 30% and shouldn't be used solely for determining regression. For more information please see the Perf Testing wiki.

@pagaur
Copy link
Contributor Author

pagaur commented Jul 19, 2019

when I try to add data-nav on autofill props, compiler gives me following error:
error TS2322: Type '{ 'data-nav': boolean; }' is not assignable to type 'IAutofillProps'.
Object literal may only specify known properties, and ''data-nav'' does not exist in type 'IAutofillProps'.

@joschect
Copy link
Contributor

@pagaur Per Jason's comment this is an issue with typescript, if you type those props as any they will work and should get passed down to the input. See the screenshot attached
image

@pagaur
Copy link
Contributor Author

pagaur commented Jul 19, 2019

cool, I will remove the autoFillCutomProps from the CL, does other changes looks fine?

@aneeshack4
Copy link
Contributor

We could potentially do what ComboBox has done with Autofill in order to let InputHTMLAttributes to be mixed in but what you have right now is better because it's less confusing what the root element is this way.

@joschect joschect merged commit 8381a57 into microsoft:master Jul 22, 2019
@msft-github-bot
Copy link
Contributor

🎉[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@microsoft microsoft locked as resolved and limited conversation to collaborators Sep 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support to set custom attributes on Spin button or ComboBox components
7 participants