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: email field on add person #5097

Merged
merged 8 commits into from
May 23, 2021

Conversation

iloveitaly
Copy link

Adds an email field to the 'add person' screen. I described why this is a helpful feature and it also paves the way for #5083.

I don't think it makes sense to add this feature to the API at it's strictly a UX improvement and doesn't mutate the data model structure.

cc @djaiss

image

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.

Front-end changes

  • If you change the UI, make sure to ask repositories administrators first about your changes by pinging djaiss or asbiin in this PR.
  • Screenshots are included if the PR changes the UI.
  • Front-end tests have been written with Cypress.

Backend/models changes

  • The API has been updated.
  • API's documentation has been added by submitting a pull request in the marketing website repository.
  • Tests have been added for the new code.
  • If you change a model, make sure the SetupTest file is updated. We need seeders to develop locally and generate fake data.

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

Thanks @iloveitaly. I think this is a nice addition to the product!

@iloveitaly iloveitaly force-pushed the 5078-email-field-on-add-person branch from 0427a43 to b793ce7 Compare April 25, 2021 19:43
@iloveitaly
Copy link
Author

@djaiss Great! Looks like I didn't update the assets correctly. Rebased on master and ran yarn run lint:all && yarn run prod. Mind approving the tests to run?

@asbiin
Copy link
Member

asbiin commented May 4, 2021

@iloveitaly you can rebase/merge fix master, where the assets are not needed anymore in the repository.

@iloveitaly iloveitaly force-pushed the 5078-email-field-on-add-person branch from 4c2941d to ab957a6 Compare May 12, 2021 19:02
@iloveitaly
Copy link
Author

@asbiin I think this one is ready to go! Could you take a look?

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.

Great work! Some slight changes to made:

.env.example Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
app/Services/Contact/Contact/CreateContact.php Outdated Show resolved Hide resolved
tests/Unit/Services/Contact/Contact/CreateContactTest.php Outdated Show resolved Hide resolved
@iloveitaly iloveitaly force-pushed the 5078-email-field-on-add-person branch from 95bcc17 to ed0dc7e Compare May 17, 2021 14:36
@iloveitaly
Copy link
Author

@asbiin removed changelog entry and applied suggested changes. This should be good to go!

Co-authored-by: Alexis Saettler <alexis@saettler.org>
@iloveitaly iloveitaly force-pushed the 5078-email-field-on-add-person branch from ed0dc7e to 81d33b2 Compare May 21, 2021 02:08
@iloveitaly
Copy link
Author

@asbiin removed the .env file comment. Take another look!

@asbiin asbiin merged commit 2392afc into monicahq:master May 23, 2021
@asbiin
Copy link
Member

asbiin commented May 23, 2021

Thank you @iloveitaly !

@github-actions
Copy link

🎉 This PR is included in version 3.1.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 Jun 24, 2022
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.

None yet

3 participants