-
Notifications
You must be signed in to change notification settings - Fork 174
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(2464): exclude contact from birthday calendar #3009
feat(2464): exclude contact from birthday calendar #3009
Conversation
Codecov ReportBase: 31.40% // Head: 67.96% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #3009 +/- ##
=============================================
+ Coverage 31.40% 67.96% +36.56%
Complexity 253 253
=============================================
Files 110 23 -87
Lines 1863 721 -1142
Branches 218 0 -218
=============================================
- Hits 585 490 -95
+ Misses 1163 231 -932
+ Partials 115 0 -115 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Is there any tests I can do on this one for the frontend ? I'm trying to run tests locally for the server side to validate what I've pushed is valid, but for the contacts side, I haven't find much resources and don't know how I could test my code. |
Hi, @miaulalala or @tcitworld, could one of you please point me to some directions on how to tests this PR for the frontend part, if tests are mandatory for this to be merged ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, thanks a lot for your contribution.
The button needs a version check because the server PR will only be working from 26 onwards.
Example:
parseInt(OC.config.version.split('.')[0]) >= 26
And then toggle it via v-if
.
I added a version check on the button as requested |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works. Thanks again for your contribution :)
There is one minor issue with the wording but otherwise we are good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
One last thing I totally forgot in my last review: Please squash all commits into one before the merge. I'm sorry for all the nitpicking. This will be my last request 🙈
Signed-off-by: Sylvain <[email protected]> Co-authored-by: Thomas Citharel <[email protected]> Co-authored-by: Richard Steinmetz <[email protected]>
I've squashed all commits into a single one, the history should be better now :) |
Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/contacts/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22 |
This should close #2464