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

[17.0] [IMP] partner_contact_gender: improve readme and icon #1851

Merged
merged 3 commits into from
Oct 16, 2024

Conversation

polchampion
Copy link

@polchampion polchampion commented Sep 30, 2024

OCA Days workshop - improving documentation by fucntionals

Copy link
Contributor

@remi-filament remi-filament left a comment

Choose a reason for hiding this comment

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

Looks good,
Thanks for this first contribution !
Maybe commits should be squashed during merge to keep history clean ?

@polchampion
Copy link
Author

@gurneyalex Do you think this can be merged?

@polchampion
Copy link
Author

@TumbaoJu My OCA Days PR seems stuck... how should I proceed?

@houssine78
Copy link

@polchampion you're missing some approvals to allow the merge. Could squash your commits into one as asked by Rémi? ping me back when it's done I'll approve the PR.

@TumbaoJu
Copy link

@remi-filament @houssine78 : As discussed during the OCA Days, the commit squash is not required for Read me PR.

Non-technical persons wrote those PR and have not the knowledge to be able to squash the commits so we agreed that for Read me PR, it would not be mandatory.

Thank you in advance for your help.
@polchampion : FYI

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@houssine78
Copy link

Hi @TumbaoJu ,

Thanks for the info, I wasn't aware of this.

Copy link

@houssine78 houssine78 left a comment

Choose a reason for hiding this comment

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

small non technical changes.

@houssine78
Copy link

Hi @pedrobaeza this PR is ready for merging

@pedrobaeza pedrobaeza added this to the 17.0 milestone Oct 16, 2024
@rousseldenis
Copy link
Contributor

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 17.0-ocabot-merge-pr-1851-by-rousseldenis-bump-patch, awaiting test results.

@pedrobaeza
Copy link
Member

Please squash both icon commits into one. About the icon itself, I understand the motivations for changing it, but I think the icon is not self-representative. It can be easily confused with a country flag, and it's also not incorporating a contact representation. It can be at least a merge of the avatar icon (like https://github.com/OCA/partner-contact/blob/16.0/partner_contact_lang/static/description/icon.png) with the flag in one corner.

@OCA-git-bot OCA-git-bot merged commit c8b310b into OCA:17.0 Oct 16, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at b6be5b7. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants