Skip to content
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

Feature/domain autocomplete getters #368

Merged
merged 9 commits into from
Jul 26, 2021

Conversation

just-at-uber
Copy link
Contributor

@just-at-uber just-at-uber commented Jul 23, 2021

Added

  • DomainAutocomplete getters
  • combine helper which takes data & callbacks as arguments which will pass data to first callback then passes output to next callback until the final callback returns the final output (similar to how middleware works).

@just-at-uber just-at-uber changed the title Feature/domain search getters Feature/domain autocomplete getters Jul 23, 2021
@just-at-uber just-at-uber marked this pull request as ready for review July 23, 2021 21:57
@just-at-uber just-at-uber requested a review from a team July 23, 2021 21:57
// THE SOFTWARE.

const combineDomainList = ({ domainList, visitedDomainList }) => {
const domainListUuidList = domainList.map(domain => domain.domainInfo.uuid);
Copy link
Member

Choose a reason for hiding this comment

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

how big do these lists get? are they prior to the 15-limit or after?
if it's prior to that limit, probably worth switching this to be a map lookup - 100s might be visibly slow to do repeated lookups on a list like this.

Copy link
Contributor Author

@just-at-uber just-at-uber Jul 26, 2021

Choose a reason for hiding this comment

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

Right now there is no cap on visitedDomainList so this can grow infinitely. I will change the action in the next PR (#369) to cap out to a max of 15.

Copy link
Member

@Groxx Groxx left a comment

Choose a reason for hiding this comment

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

Seems alright, though I admit I don't follow all the pieces involved.
(e.g. all the getters' source data / etc. should be pretty obvious if it works while testing locally tho)

@just-at-uber just-at-uber merged commit 9e1cc44 into master Jul 26, 2021
@just-at-uber just-at-uber deleted the feature/domain-search-getters branch July 26, 2021 23:24
@just-at-uber just-at-uber mentioned this pull request Jul 28, 2021
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.

3 participants