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: order introductions contact list by first and last name #5102

Merged
merged 11 commits into from
Jan 1, 2022

Conversation

iloveitaly
Copy link

It's feels much more natural to me to order the introductions list by first and last name.

Checklist

Before submitting the PR

  • Read the CONTRIBUTING document before submitting your PR.
  • If the PR is related to an issue or fix one, don't forget to indicate it.
  • Create your PR as draft if it is not final yet. Mark it as ready... when it’s ready. Otherwise the PR will be considered complete and rejected if it's not working.

General checks

  • Make sure that the change you propose is the smallest possible.
  • The name of the PR should follow the conventional commits guideline that the project follows.

Other tasks

  • CHANGELOG entry added, if necessary, under UNRELEASED.
  • CONTRIBUTORS entry added, if necessary.
  • If it's relevant and worth mentioning, create a changelog entry for this change. The changelog entry will appear inside the UI for all users to see. To know if your change is worth the creation of a changelog entry, read the documentation.
  • Don't forget to ask for a free account on https://monicahq.com as anyone who contributes can request a free account.

@djaiss
Copy link
Member

djaiss commented Apr 24, 2021

Hi @iloveitaly and thanks for the PR.

To you it might feel more natural, but in other cultures, it might not.

This is why we introduced the column called name_order in the users table that define how people want their contact to be sorted.

I think we should use this in this case, what do you think @asbiin ?

@iloveitaly
Copy link
Author

Ah, great, didn't realize there was an option for that. It sounds like we should add a switch similar to this one to add 2-3 orderBys to the query. I'm not that familiar with the codebase—where would be the best place for this helper?

@djaiss
Copy link
Member

djaiss commented Apr 27, 2021

Ah, great, didn't realize there was an option for that. It sounds like we should add a switch similar to this one to add 2-3 orderBys to the query. I'm not that familiar with the codebase—where would be the best place for this helper?

Good question.
We might need to use this in other places too. So I would perhaps create a ContactHelper with the right method name in it, like getNameInUserOrder or something like this (yes, naming is hard).

@iloveitaly iloveitaly force-pushed the order-introductions-by-name branch from 6eba0b7 to a1dda45 Compare May 12, 2021 21:33
@iloveitaly
Copy link
Author

@djaiss added a helper to execute this logic. Take a look!

@iloveitaly
Copy link
Author

@asbiin Any thoughts on how to fix the typing error? Builder as a param + return type seems to return an error since HasMany is passed in.

Copy link
Member

@asbiin asbiin left a comment

Choose a reason for hiding this comment

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

phpstan is complaining, and I think it would be better to use a scope

app/Helpers/ContactHelper.php Outdated Show resolved Hide resolved
@asbiin asbiin merged commit 6ff0738 into monicahq:master Jan 1, 2022
@github-actions
Copy link

🎉 This PR is included in version 3.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

This pull request has been automatically locked since there
has not been any recent activity after it was closed.
Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants