Skip to content

Conversation

@khanhtc1202
Copy link
Member

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Sort the application suggestion names in application filter form

@khanhtc1202 khanhtc1202 enabled auto-merge (squash) June 8, 2022 08:56
@@ -1 +1,2 @@
export const uniqueArray = <T>(arr: Array<T>): Array<T> => [...new Set(arr)];
export const uniqueArray = <T>(arr: Array<T>): Array<T> =>
[...new Set(arr)].sort();
Copy link
Member

Choose a reason for hiding this comment

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

It seems that you changed at the shared place.
Will this affect all places where this function is being used? Is that ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, I confirmed that this one only is used in related places we want. And besides, having a sorted set is a good point as well, so I think it's okay to make change like this.

Copy link
Member

Choose a reason for hiding this comment

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

If that how about renaming it to sortedArray to be precise.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, I rename it as sortedSet 🙆‍♀️

knanao
knanao previously approved these changes Jun 8, 2022
Copy link
Member

@knanao knanao left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

@nghialv nghialv left a comment

Choose a reason for hiding this comment

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

Thank you.

@khanhtc1202
Copy link
Member Author

@knanao sorry but could you re-approval this PR 🙏

Copy link
Member

@knanao knanao left a comment

Choose a reason for hiding this comment

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

Sure, Here you go!

@khanhtc1202 khanhtc1202 merged commit b0932de into master Jun 8, 2022
@khanhtc1202 khanhtc1202 deleted the sort-application-name branch June 8, 2022 10:08
@github-actions github-actions bot mentioned this pull request Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants