Skip to content

guillaume's changes#1

Open
MarkCBall wants to merge 1 commit intoCP2053:masterfrom
Rome-Blockchain-Labs:guillaume
Open

guillaume's changes#1
MarkCBall wants to merge 1 commit intoCP2053:masterfrom
Rome-Blockchain-Labs:guillaume

Conversation

@MarkCBall
Copy link

No description provided.

searchNetworks.push(networkName);
}

// Sets the new value for "searchNetworks" using a "Set" to *lazy* clean duplicates; who knows?
Copy link
Author

Choose a reason for hiding this comment

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

I like lodash and we have used it a lot elsewhere.
https://lodash.com/docs/4.17.15#uniq

clearTimeout(searchDelayer);

// Sets the new search delayer and the new searc text value.
setSearchDelayer(setTimeout(value => setSearchText(value), process.env.REACT_APP_SEARCH_INPUT_TEXT_DELAYER, searchText));
Copy link
Author

Choose a reason for hiding this comment

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

i don't know if this will work, there could be a race condition in here.

Have you considered something like
https://www.npmjs.com/package/debounce

Copy link
Owner

Choose a reason for hiding this comment

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

There shouldn't be a race condition with the variable use to cancel the previous setTimeout, unless something weird is done while React compiles. Also looked at the source of debounce and it's doing that same thing.

type="checkbox"
checked={searchExchanges.includes(exchangeName)}
onChange={() => {
let filter;
Copy link
Author

Choose a reason for hiding this comment

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

this seems overly complicated for a potential future we might not have.

did you check if onChange passes an arg to the callback that can be used in liew of the toggle you are trying to set up?

Then you could do something like

onChange={(e)=>{
setSearchExchanges(searchExchanges=>({
  {...searchExchanges,
  [exchangeName]:e.target.checked
  }
}))
}}

Copy link
Author

Choose a reason for hiding this comment

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

this functional version of setState also avoids any possible race conditions; it can't get out of sync with the actual html element

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.

2 participants