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

Return raw feature data from workers #225

Merged

Conversation

Clebal
Copy link
Contributor

@Clebal Clebal commented Nov 17, 2021

Story details: https://app.shortcut.com/cartoteam/story/192546

Possible questions

  • Why have you modified buildFeatureFilter fn?
    It's a minor improvement to avoid calling Object.keys for each feature

  • Why that new applyFilters fn in core?
    I think this can be used in advanced use cases.

  • Do you think that sorting might be part of core?
    I don't think so because it's just used in workers, and I don't think it can be used in other parts of C4R or even that C4R have to provide any kind of sorting out of the main think. Idk. Let me know what do you think.

  • What's sortBy structure?
    This can be checked here.
    sortBy accepts:

    1. An string (a column) sortBy: 'column_1'
    2. Array of strings (multiple columns) sortBy: ['column_1', 'column_2']
    3. An array where each element follows thenBy structure. Passing each child array as arguments to firstBy/thenBy functions. Thank to this we are able to specify direction for each column or ignoring cases or any other feature of thenBy.

Example of the third option:

sortBy: [
    ['column_1', 'desc'],
    ['column_2', { direction: 'asc', ignoreCase:true }]
]
  • I saw that thenBy accepts function? Does sortBy accepts function?
    Nope, functions cannot be cloned to workers.

  • If I indicate direction using thenBy structure and I also indicate sortByDirection. Which is applied?
    thenBy structure has priority since it's more specific. If sorting is applied by multiple column and some doesn't indicate direction, sortByDirection is used (which value by default is 'asc').

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #192546: Return raw feature data from workers.

@coveralls
Copy link
Collaborator

coveralls commented Nov 17, 2021

Pull Request Test Coverage Report for Build 1536090354

  • 2 of 3 (66.67%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 69.058%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/react-core/src/filters/Filter.js 2 3 66.67%
Totals Coverage Status
Change from base Build 1526975873: -0.1%
Covered Lines: 902
Relevant Lines: 1220

💛 - Coveralls

Copy link
Contributor

@Josmorsot Josmorsot left a comment

Choose a reason for hiding this comment

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

Nice work! Just minor suggestions :)

Copy link
Contributor

@Josmorsot Josmorsot left a comment

Choose a reason for hiding this comment

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

LGTM, wait for double review before merge 🚀

@Clebal
Copy link
Contributor Author

Clebal commented Nov 29, 2021

@VictorVelarde @padawannn

Co-authored-by: Víctor Velarde <victor.velarde@gmail.com>
Copy link
Contributor

@VictorVelarde VictorVelarde left a comment

Choose a reason for hiding this comment

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

Just small comments, but it looks like a nice approach to me. Remember to add a changelog and rebase master, please

Copy link
Contributor

@VictorVelarde VictorVelarde left a comment

Choose a reason for hiding this comment

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

Nice job. 🚀

@VictorVelarde VictorVelarde merged commit c6d5cff into master Dec 4, 2021
@VictorVelarde VictorVelarde deleted the feature/sc-192546/return-raw-feature-data-from-workers branch December 4, 2021 15:17
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.

4 participants