-
Notifications
You must be signed in to change notification settings - Fork 686
Date picker and time picker color by theme #3558
Date picker and time picker color by theme #3558
Conversation
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.
Fix the CI and I'll review, in the meantime let our contributors review 🙏
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.
Left some comments above
you want me to do a reformat for detekt? When I do this, more than 200 files appear :) Even if I edit where I made the changes it still gives error. |
Read this https://github.com/Ivy-Apps/ivy-wallet/blob/main/docs/CI-Troubleshooting.md |
@oguzsout I can fix the detekt issues if you want. |
Yes, I forgot to check here, I will review it again and make the change 🙏 |
Thank you very much @shamim-emon , I'm on it now. I'll write to you if I get stuck 🙃 |
Also, make sure to read our Contribution Guidelines. |
Since the number of lines of the onCreate method exceeds the restriction specified in rule Detekt, I created the setupApp function. I opened and tested the App and it did not cause any problems. @ILIYANGERMANOV @shamim-emon |
Sounds good. If the Detekt error is in legacy code, feel free to suppres using an annotation |
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.
@oguzsout your PR description is missing the section where you have to mention which issue is this PR closing. For reference adding a screenshot below
@oguzsout the issue that you're working on isn't assigned to you. Did you read the contributor guidelines? Lucky for you the assignee haven't completed it in 3 weeks |
@oguzsout read the Contribution Guidelines and assign it to yourself |
To fix the PR description check you need to push a commit unfortunately |
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.
Found an issue with edge-to-edge
shared/ui/core/src/main/java/com/ivy/ui/time/impl/AndroidDateTimePicker.kt
Outdated
Show resolved
Hide resolved
Since that person didn't do it, I wanted to do it, but I couldn't take it upon myself. |
In that case, you need to ping me to unassign and then you can take it. We should probably add CI automation for unassigning, as well |
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! Ping me to merge it after the CI pass 🚀
Pull request (PR) checklist
Please check if your pull request fulfills the following requirements:
Description
What's changed?
Risk factors
...
Does this PR close any GitHub issues? (do not delete)
Troubleshooting GitHub Actions (CI) failures ❌
Pull request checks failing? Read our CI Troubleshooting guide.