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

feat(get-medias/sort): add option to customize sort on getMedias #37

Merged
merged 4 commits into from
Feb 18, 2023

Conversation

matheusdavidson
Copy link
Contributor

This PR will add the ability to sort getMedias using custom keys and ascending.
#36

@nkalupahana
Copy link
Collaborator

Thank you so much for your contribution! Two quick things before I review:

  • Could you make the two options in MediaSort optional? Users may want to specify one but not the other.
  • Can you have multiple sort descriptors at a time? If so, it would be great if you could add that functionality; I don't think it would be too hard to add.

Again, thank you for contributing! We really appreciate it.

@stewones
Copy link
Member

stewones commented Feb 15, 2023

Thank you so much for your contribution! Two quick things before I review:

  • Could you make the two options in MediaSort optional? Users may want to specify one but not the other.
  • Can you have multiple sort descriptors at a time? If so, it would be great if you could add that functionality; I don't think it would be too hard to add.

Again, thank you for contributing! We really appreciate it.

agree. and would make it clear through the doc block what the default value is for both options.

for point 2 I think it could potentially hold off merging this PR. maybe just define a broader type definition that can support multiple sort descriptors in the future without breaking the proposed feature.

@stewones stewones marked this pull request as draft February 15, 2023 21:57
@stewones
Copy link
Member

looking better at it, I think what @nkalupahana is proposing is to pass an array of keys instead of a string. So I tend to agree and we stay consistent with iOS API.

@matheusdavidson
Copy link
Contributor Author

Hi @nkalupahana and @stewones, thanks for reviewing.

I agree with both points but on point 1 the key can't be optional after implementing point 2, because being an array to enable multiple sort options, the key is the only important part inside the array, for example, an array of [{ ascending: false }, { ascending: true }] wouldn't make sense, but [{ key: creationDate }, { key: isFavorite }] makes sense.

So here what I did:

  • make sort accept: string | MediaSort[]
  • when sort is a string, it will be used as a key and default ascending to false.
  • when sort is an array, it will loop and append to sortDescriptors, here the key is required and ascending optional, if one is not provided true is used by default.

src/definitions.ts Outdated Show resolved Hide resolved
@stewones stewones marked this pull request as ready for review February 16, 2023 01:43
@matheusdavidson matheusdavidson requested review from stewones and removed request for nkalupahana February 16, 2023 01:43
@stewones stewones requested a review from nkalupahana February 16, 2023 01:47
Copy link
Member

@stewones stewones left a comment

Choose a reason for hiding this comment

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

good job, LGTM.

@matheusdavidson
Copy link
Contributor Author

Sorry @nkalupahana, didn't meant to remove you from review, github only gave me an option to ask @stewones, please review it as well

@nkalupahana
Copy link
Collaborator

Looks good! Could you remove the debug prints? Other than that, I think we're good to get this merged :) thanks for your contribution!

@matheusdavidson
Copy link
Contributor Author

@nkalupahana, @stewones, removed debug print. Should be good to go now.

@nkalupahana nkalupahana merged commit 187b253 into capacitor-community:master Feb 18, 2023
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