Skip to content

Prefer const for ExtendedPicker, FloatingPicker, and SelectedItemsList#4511

Merged
amyngu merged 3 commits intomicrosoft:masterfrom
amyngu:amyngu/preferConst
Apr 12, 2018
Merged

Prefer const for ExtendedPicker, FloatingPicker, and SelectedItemsList#4511
amyngu merged 3 commits intomicrosoft:masterfrom
amyngu:amyngu/preferConst

Conversation

@amyngu
Copy link
Copy Markdown
Contributor

@amyngu amyngu commented Apr 10, 2018 •
edited
Loading

Pull request checklist

  • Include a change request file using $ npm run change

Description of changes

  • Prefer const to lets for ExtendedPicker, FloatingPicker, and SelectedItemsList
  • Remove unused props in suggestions, since consumer has full use of header and footers, I don't think its necessary for the control to show is searching or results footer

@amyngu amyngu requested review from joschect and micahgodbolt April 10, 2018 23:12
micahgodbolt
micahgodbolt reviewed Apr 10, 2018
View reviewed changes
packages/experiments/src/components/FloatingPicker/Suggestions/Suggestions.types.ts
* The CSS classname of the suggestions list.
*/
className?: string;
/**
Copy link
Copy Markdown
Member

@micahgodbolt micahgodbolt Apr 10, 2018

Choose a reason for hiding this comment

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

These interface changes aren't let => const. just wanted to make sure they were supposed to be in here.

Copy link
Copy Markdown
Contributor Author

@amyngu amyngu Apr 10, 2018

Choose a reason for hiding this comment

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

Yes, sorry I forgot I made these changes as well (was copy and pasting). These are just a removal of unused properties. I'll make sure I have @joschect sign off on these changes, as hes been following my initiative here.

Copy link
Copy Markdown
Contributor

@joschect joschect Apr 12, 2018

Choose a reason for hiding this comment

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

@micahgodbolt I thought since these are experiments, no one should be using them and we should be able to do whatever when it comes to interfaces. Right?

Copy link
Copy Markdown
Member

@micahgodbolt micahgodbolt Apr 12, 2018

Choose a reason for hiding this comment

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

Amy is prepping this component for a PR into OUFR, and I had her split it up into let/const changes...I saw this change as well and wanted to make sure it was intentional.

@amyngu amyngu closed this Apr 12, 2018
@amyngu amyngu reopened this Apr 12, 2018
@amyngu amyngu merged commit 7a5e2c3 into microsoft:master Apr 12, 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

@micahgodbolt micahgodbolt micahgodbolt left review comments

+1 more reviewer

@joschect joschect joschect approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@amyngu @micahgodbolt @joschect