Skip to content

Conversation

@McDoyen
Copy link

@McDoyen McDoyen commented Nov 26, 2018

  • Dropdown type ahead

@mendixlabs mendixlabs deleted a comment from McDoyen Nov 27, 2018
Copy link

@diego-antonelli diego-antonelli left a 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: [] });

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: [] });

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) {

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();

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,

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) {
Copy link

@diego-antonelli diego-antonelli Nov 27, 2018

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) {

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) => {
Copy link

@diego-antonelli diego-antonelli Nov 27, 2018

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:

  1. the forEach is never stopped when it finds the equivalent to the value
  2. 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

@McDoyen McDoyen force-pushed the feature/reference-set branch 2 times, most recently from 14a6ff9 to 55b9f0b Compare November 30, 2018 12:09
@McDoyen McDoyen force-pushed the feature/reference-set branch from 55b9f0b to 4e28a45 Compare December 3, 2018 13:07
@McDoyen McDoyen force-pushed the feature/reference-set branch from 6af20b8 to 6cea6bd Compare December 6, 2018 14:28
@McDoyen McDoyen force-pushed the feature/reference-set branch from 6cea6bd to 79448b0 Compare December 6, 2018 14:34
@McDoyen McDoyen force-pushed the feature/reference-set branch from cabca2e to 7b487f0 Compare December 7, 2018 14:18
@McDoyen McDoyen force-pushed the feature/reference-set branch 3 times, most recently from b0eccb5 to a38e3e2 Compare December 14, 2018 06:11
@McDoyen McDoyen force-pushed the feature/reference-set branch from a38e3e2 to a8299ed Compare December 14, 2018 07:00
@McDoyen McDoyen force-pushed the feature/reference-set branch 4 times, most recently from 35e5747 to 61f3673 Compare December 19, 2018 09:09
@McDoyen McDoyen force-pushed the feature/reference-set branch 2 times, most recently from 6f325a7 to 650c76f Compare December 21, 2018 12:44
@McDoyen McDoyen force-pushed the feature/reference-set branch from 650c76f to 7349ba7 Compare December 21, 2018 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants