-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[BUG] List filtering not working after rotating device #4467
[BUG] List filtering not working after rotating device #4467
Conversation
@JuancaG05 please review this one. |
44642c7
to
0361c2e
Compare
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.
Some comments here @AwaisKhan128! BTW, when you push new commits fixing what I commented here, please do it with conventional commits naming as well. There is already 1 commit (the one to update 4464
) that doesn't follow the convention 😅. I encourage you to remove those commits with an interactive rebase and add a new one that includes the file 4467
correctly 👍
Alright. |
@JuancaG05 Could you please have a look now? |
Hi @AwaisKhan128, friendly reminder that you should use conventional commits for naming, there are several commits not following that convention 😃 |
@JuancaG05 I did only two commits this time. Which one you are talking about? |
Hi @AwaisKhan128! The code now is in perfect status to be merged, but if you realise, we have 14 commits in this PR (you can check them in the
Also, the revert ones are not necessary. Although the code is in a good status, if we merge this PR, all of these 14 commits will be merged, and in addition to not following the conventional commits, some of them are not useful at all because the changes performed in them were already removed (for example, What you can do here to leave it in a good status, is removing the unnecessary commits with an interactive rebase. A quick explanation so that you learn for a future too:
The number that goes together with
After this, you should have in this PR only the commits we want and will be ready to be moved to QA and if approved, finally merged 🤠 |
f7f39fb
to
19ba863
Compare
@JuancaG05 Could you please have a look on it? |
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.
Cool!! Great job @AwaisKhan128!! 🥇 Thanks a lot for your work, let's move this to QA to give the final approval 😃
e10c458
to
2f32b08
Compare
@jesmrec let's QA here the configuration changes in
|
QA checks
Devices: Xiaomi Redmi 13 |
@AwaisKhan128 thanks a lot for your contribution! you did many attempts but finally it is ready to merge and become part of the product. I hope you enjoyed the contribution and learnt something new! Wish to see you in other contributions!! |
Related Issues
App:
ReleaseNotesViewModel.kt
creating a newReleaseNote()
with String resources (if required)QA