remove back button if fragmented #16582
Conversation
|
Could you provide a screenshot comparison? |
There was a problem hiding this comment.
I'm fine with the code.
As David noted, screenshot would be helpful.
But I'd go further, and really request you to rewrite your commit message. This commit seems a great writing exercice.
There are two kind of readers you want to have in mind when writing your commit message. The reviewers, and some other dev that try to understand why the code is the way it is years later. (Probably to fix a bug)
So, when this future reader will see the one line description, they should know as much as possible.
You can state that it's for large screen only. That the button had no side effect.
Then, with more details, you can state that it was a bug that existed for a long time, unnoticed.
That the button is useful on small screen when the fragment appears on a separate screen, and is not useful when the fragment appears near the deck-picker, where there is nowhere to go back to, so we could remove the button.
In particular, you wrote
This commit hides back button if it is fragmented
It seems that the button is fragmented. This is not the case. Please state explicitly that you consider the case where the deck picker view is fragmented, i.e. big screen. Explain also which "back button" is hidden. The one that appeared without purpose.
Instead of "hidden", you may state that you remove it. From the point of view of the user, the button just does not appear. It does not exists anymore. There is no sense in stating that it's hidden, which would imply it's still here.
Finally, "fragmented" may not be the best word here. This word is specific to our codebase. From the user point of view, what matter is that the fragment appears simultaneously with the deck picker. So we can't go "back" to the deck picker.
If you stated this in your commit message, it would be quite simpler to see the commit makes total sense and should be accepted. And the future reader could understand whether this line is related or not to a potential bug they have with some back issue years later
| @@ -363,6 +363,8 @@ class StudyOptionsFragment : Fragment(), ChangeManager.Subscriber, Toolbar.OnMen | |||
| icon!!.isAutoMirrored = true | |||
| toolbar!!.navigationIcon = icon | |||
| toolbar!!.setNavigationOnClickListener { (activity as AnkiActivity).finish() } | |||
| } else { | |||
There was a problem hiding this comment.
I'd consider inversing if and else. So that the sentence can be read as "if it is fragmented" instead of the longer "if it is not fragmented".
| @@ -363,6 +363,8 @@ class StudyOptionsFragment : Fragment(), ChangeManager.Subscriber, Toolbar.OnMen | |||
| icon!!.isAutoMirrored = true | |||
| toolbar!!.navigationIcon = icon | |||
| toolbar!!.setNavigationOnClickListener { (activity as AnkiActivity).finish() } | |||
| } else { | |||
| toolbar!!.navigationIcon = null | |||
There was a problem hiding this comment.
Please add a comment explaining why it is done.
I don't expect it's clear to most reader of the codebase that there is no reason for the back button.
So, please explain that, when the fragment appear near the deck picker, the "back" button had no purpose, so should be removed
This bug existed for a long time which was unnoticed. Back button is useful on small screen when the fragment appears on a separate screen, and is not useful when the fragment appears near the deck-picker, where there is nowhere to go back to, so we could remove the button
SanjaySargam
force-pushed
the
remove-back-button
branch
from
925e933 to
130efa4
Compare
david-allison
added
the
Needs Second Approval
lukstbit
added
the
GSoC
github-actions
bot
removed
the
Needs Second Approval
|
Hi there @SanjaySargam! This is the OpenCollective Notice for PRs merged from 2024-06-01 through 2024-06-30 If you are interested in compensation for this work, the process with details is here: Important PLEASE NOTE: The process was updated in August 2024. Re-read the Payment Process page if you have not already. We only post one comment per person per month to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for this month Please understand that our monthly budget is never guaranteed to cover all claims - the cap on payments-per-person may be lower, but we try to make our process as fair and transparent as possible, we just need your understanding. Thanks! |
Purpose / Description
Hide the back button in StudyOptionsFragment when fragmented
Approach
set toolbar navigation icon to null
How Has This Been Tested?
Physical Device ( ChromeBook )
Checklist
Please, go through these checks before submitting the PR.