Refund by items: Number picker dialog - Step 1#1621
Refund by items: Number picker dialog - Step 1#16210nko wants to merge 5 commits intofeature/refund-by-items-masterfrom
Conversation
|
You can test the changes on this Pull Request by downloading the APK here. |
AmandaRiu
left a comment
There was a problem hiding this comment.
@0nko I looked over the code and left a nitpick for your review. I know this is just a subset of changes, but it'd be really helpful to have the following added to the PR so I can properly review it:
- A screenshot of how the component should look from the designs
- A snippet of code I can insert somewhere in the app to open the new number picker dialog successfully. I could add it to the "search" button in the order list or something. Just so I can see how it works and make sure it's doing what it's supposed to be doing. The snippet should include a callback so I can test that the selected number is getting properly returned to the caller.
| } | ||
|
|
||
| override fun onDismiss(dialog: DialogInterface) { | ||
| // TODO: android.app.Fragment is deprecated since Android P. |
There was a problem hiding this comment.
np: Is this TODO valid? You're using the androidx version which should automatically be using the support library code (since it's the recommended way of interacting with the library going forward).
There was a problem hiding this comment.
This isn't valid. This got copied from a different code I reused. I eventually removed the comment.
Thanks for the review! As I mentioned above, this is just an intermediate PR. I guess I should have just kept the dialog things together. I just tried to break up the number of changes to be reviewed. You can inspect it and test the functionality in #1622. The whole thing is being merged into a feature branch, which will contain everything before the final merge to |
|
I've thought about it and it's probably better to have the code in the same PR where it can be tested, so I'm closing this in favor of #1622. |
This is the first sub-PR of Refund by items feature branch. I had to split up the original PR because it became too large to review all at once. I tried to separate it by logical blocks.
Unfortunately, this one's a bit weird because there is no way to actually test the functionality. That will be possible in one of the subsequent PRs, which will have the necessary logic for displaying it.