Fix #6145: Fix Admin PIN tablet layout - #6370
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@samarthshukla20 this PR is being marked as draft because the PR description must contain 'Fixes #' or 'Fixes part of #' for each issue the PR is changing, and each one on its own line with no other text. |
|
@samarthshukla20 this PR is being marked as draft because the PR description must contain 'Fixes #' or 'Fixes part of #' for each issue the PR is changing, and each one on its own line with no other text. |
|
Hi @samarthshukla20, if the PR is ready for review, please assign @Neer-rn for a first review pass by “@username, PTAL. Always do this when you want someone to take a look at your work. |
|
@Neer-rn Hi! @adhiamboperes suggested assigning you for a first review pass on this PR. I’m unable to add you directly in the Reviewers section, so I’m mentioning you here instead. I’ve implemented and locally verified the AdminAuthActivity tablet layout fix for #6145. Could you please take a look when you get a chance? Thank you! |
|
Unassigning @samarthshukla20 since a re-review was requested. @samarthshukla20, please make sure you have addressed all review comments. Thanks! |
Coverage ReportResultsCoverage Analysis: SKIP ⏭️ This PR did not introduce any changes to Kotlin source or test files.
|
|
@samarthshukla20 Please sign the CLA before contributing. Also I'd recommend going through this wiki if you haven't already. |
Neer-rn
left a comment
There was a problem hiding this comment.
Hi @samarthshukla20, thanks for working on this!
The fix might work on the screen from the issue. But this dimen is also used by admin_auth_activity.xml, and that screen is fine right now. On a tablet its content sits as a 360dp column in the middle, and after this change it might stretche to the full width with no margins, so the text and PIN box touch the screen edges. This is me just guessing looking at the code but you can verify this for me.
The screen in the issue is actually AdminPin, not AdminAuth. Both have the same toolbar title so it is easy to mix up. Please read this comment I think the solution to this issue is already mentioned here. Please try to work on it.
Also when you are done with your work you can re-assign me this by commenting '@username PTAL' and the oppia-bot will assign me for review you cannot manually assign me the issue as of now.
|
Unassigning @Neer-rn since the review is done. |
|
Hi @samarthshukla20, it looks like some changes were requested on this pull request by @Neer-rn. PTAL. Thanks! |
|
@Neer-rn, PTAL |
|
Hi @samarthshukla20, it looks like some changes were requested on this pull request by @Neer-rn. PTAL. Thanks! |
|
Done. I reverted the two dimens.xml changes as suggested and tested the fix again. The layout still works correctly with only the change in admin_pin_activity.xml. I also updated the PR description with before and after screenshots. @Neer-rn PTAL, thanks! |
|
Unassigning @samarthshukla20 since a re-review was requested. @samarthshukla20, please make sure you have addressed all review comments. Thanks! |
Neer-rn
left a comment
There was a problem hiding this comment.
Hi @samarthshukla20, I see no changes here, your last commit was on Aug 25, did you push your new changes?? Please make sure you address my previous comment then only re-assign me.
|
Unassigning @Neer-rn since the review is done. |
|
Hi @samarthshukla20, it looks like some changes were requested on this pull request by @Neer-rn. PTAL. Thanks! |
|
@samarthshukla20 I think you reverted the files locally but didn't commit and push them. Please look at the section of files changed still I can see 3 files changed. I can only see one single commit being pushed in total on this PR. The commit you showed me ca3a894 doesn't belong to this particular branch. If you are not sure how git and github works then I recommend you taking help of LLM to figure things out.
|
Coverage ReportResultsNumber of files assessed: 42 Passing coverageFiles with passing code coverage
Exempted coverageFiles exempted from coverage
|
|
Hi @Neer-rn, I’ve addressed your feedback. The unnecessary dimens.xml changes have been reverted and the PR now contains only the change in admin_pin_activity.xml. I also verified the updated layout on the tablet emulator and updated the PR description with before/after screenshots. PTAL, thanks! |
|
Unassigning @samarthshukla20 since a re-review was requested. @samarthshukla20, please make sure you have addressed all review comments. Thanks! |
Neer-rn
left a comment
There was a problem hiding this comment.
Hi @samarthshukla20, One small thing: your PR title and description say AdminAuthActivity, can you fix that in both places? Also the "Before" image is the same one from the issue did you just take that image from issue? Were you able to reproduce the issue or not?
|
Hi @Neer-rn, thanks! I've updated the PR title and description to refer to the Admin PIN layout instead of AdminAuthActivity. The "Before" screenshot is from the original issue report; I did not capture it myself. I verified the updated layout on the emulator after applying the fix. PTAL, thanks! |
|
@samarthshukla20, could you please add the full screenshots expected? One of the best ways for displaying multiple images is to use markdown tables. Please see #5880 for example. Please see documentation on md tables: https://www.markdownguide.org/extended-syntax/ |
|
Hi @adhiamboperes ! I wanted to provide an update regarding testing the fix. I was able to successfully make and verify the layout change earlier in my WSL environment, and the Bazel build completed successfully there. However, while cleaning up my development environment afterward, I accidentally removed that WSL setup. I attempted to recreate the environment on Windows to continue testing, including setting up the required Android SDK versions and Bazel dependencies, but the Oppia build requires several environment-specific dependencies and the setup started consuming a significant amount of storage. I therefore decided to stop recreating the environment. The code change itself is limited to changing the NestedScrollView width to 0dp, allowing it to fill the available width through its existing constraints while preserving the centered content layout. I have also included the before/after screenshots demonstrating the issue and the resulting fix. I apologize for the inconvenience caused by this situation. I hope this does not cause any issues with merging the PR, but please let me know if you would prefer any additional testing or changes from my side. Thank you for your understanding! |
Thanks for the update @samarthshukla20. I understand that configuring bazel on windows can be really complex but the setup issue does not replace the UI verification required for this PR. As this PR touches part of UI so we will need screenshots to see if the cahnges are working properly. As above mentioned by @adhiamboperes you will need to add before and after of both mobile and tablet screens in light and dark mode. However if you are having a lot of issues setting up the environment you can always file a debugging doc iincluding your OS, setup steps, exact errors/logs, and what you have tried. We will be happy to help. |




Explanation
Fixes #6145
This PR fixes the Admin PIN tablet layout where the screen background did not
stretch across the full available width, resulting in large empty spaces around
the content.
The fix changes the
NestedScrollViewwidth to0dp, allowing it to fill theavailable width using its existing start and end constraints while preserving
the existing centered content layout.
Essential Checklist
Disclosure of LLM Usage
For UI-specific PRs only
Before
The screenshot below is from the original issue report and illustrates the
reported layout problem, where the background did not cover the full available
tablet screen.
After
The updated layout was verified on the emulator. The background now fills the
available screen width while the content remains properly centered.