-
Notifications
You must be signed in to change notification settings - Fork 3
Feature/reference set #1
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
base: master
Are you sure you want to change the base?
Conversation
McDoyen
commented
Nov 26, 2018
- Dropdown type ahead
…pdown-type-ahead into tests/end-to-end
…pdown-type-ahead into tests/end-to-end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider the new comments. Specially the file DropdownReferenceSet.ts line 142 to 153, because looks like a performance issue.
| } else { | ||
| this.getSelectedValue(this.props); | ||
|
|
||
| return Promise.resolve({ options: [] }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is not necessary. The last return will be called
| } else { | ||
| this.getSelectedValues(this.props); | ||
|
|
||
| return Promise.resolve({ options: [] }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is not necessary. The last return will be called
| const dropdownElement = dropdown[0] as HTMLElement; | ||
| if (dropdownElement && dropdownElement.style.visibility !== "visible") { | ||
| const dropdownDimensions = dropdown[0].getBoundingClientRect(); | ||
| if (dropdownElement && dropdownDimensions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dropdownElement is already checked at line 102, if you just use if (dropdownDimensions){ it will have the same effect
| const dropdown = document.getElementsByClassName("Select-menu-outer"); | ||
| const dropdownElement = dropdown[0] as HTMLElement; | ||
| if (dropdownElement && dropdownElement.style.visibility !== "visible") { | ||
| const dropdownDimensions = dropdown[0].getBoundingClientRect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dropdown[0] is equivalent to dropdownElement. Use const dropdownDimensions = dropdownElement.getBoundingClientRect();
| .then((newValue: any) => { | ||
| if (newValue) { | ||
| this.setState({ | ||
| isClearable: this.props.isClearable ? true : false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use !!this.props.isClearable instead of this.props.isClearable ? true : false
| const dropdownElement = dropdown[0] as HTMLElement; | ||
| if (dropdownElement && dropdownElement.style.visibility !== "visible") { | ||
| const dropdownDimensions = dropdownElement.getBoundingClientRect(); | ||
| if (dropdownElement && dropdownDimensions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dropdownElement is already checked at line 117. Use if (dropdownDimensions) { instead
|
|
||
| if (this.props.selectedValue.length > 0) { | ||
| formatedOptions = this.props.selectedValue.map((selectedGuid: any) => { | ||
| if (this.props.selectedValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line will always be true, because it's inside the map of itself! It needs to be merged with line 141.
Suggestion: remove the line 143 and at line 141 use: if (this.props.selectedValue && this.props.selectedValue.length > 0) {
| if (this.props.selectedValue.length > 0) { | ||
| formatedOptions = this.props.selectedValue.map((selectedGuid: any) => { | ||
| if (this.props.selectedValue) { | ||
| this.props.selectedValue.forEach((dataObject: any) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@McDoyen maybe I'm wrong, but this block of code (lines 142 to 153), looks like a forEach in the same list of objects to verify if the value is equal and then return the label, but here we have 2 problems:
- the forEach is never stopped when it finds the equivalent to the value
- the number of interactions is this.props.selectedValue.length ^ 2
Instead of that, if is just to return an array of selected labels, use this:
formatedOptions = this.props.selectedValue.map((selectedGuid: any) => selectedGuid.label);
If my understanding is wrong, add a return; at line 148
14a6ff9 to
55b9f0b
Compare
55b9f0b to
4e28a45
Compare
6af20b8 to
6cea6bd
Compare
6cea6bd to
79448b0
Compare
cabca2e to
7b487f0
Compare
b0eccb5 to
a38e3e2
Compare
a38e3e2 to
a8299ed
Compare
35e5747 to
61f3673
Compare
6f325a7 to
650c76f
Compare
650c76f to
7349ba7
Compare