Skip to content
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

Fixes #2399: Reformatting required in dimension file for better structure #5168

Merged

Conversation

aayushimathur6
Copy link
Collaborator

@aayushimathur6 aayushimathur6 commented Oct 1, 2023

Explanation

Fixes #2399

This PR has restructured the dimens.xml files. The file is too large so just rearranged the dimensions to form the groups on the base of their name, so that we find all the the dimensions related to a particular category at once with ease.

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

  • Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
  • For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
  • Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
  • Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

@aayushimathur6 aayushimathur6 requested a review from a team as a code owner October 1, 2023 12:42
@aayushimathur6 aayushimathur6 changed the title Reformatted dimens xml file for better structure Fixes #2399: Reformatting required in dimension file for better structure Oct 1, 2023
@aayushimathur6 aayushimathur6 requested review from adhiamboperes and removed request for adhiamboperes October 1, 2023 17:19
@aayushimathur6
Copy link
Collaborator Author

aayushimathur6 commented Oct 1, 2023

@adhiamboperes PTAL, Thanks

Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for organizing this file @aayushimathur6!

I propose that we keep the values categorized by screen name, and we don't have to break the screen dimens down into further categories because that will make the file more difficult to maintain.

app/src/main/res/values/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/values/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/values/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/values/dimens.xml Outdated Show resolved Hide resolved
@oppiabot
Copy link

oppiabot bot commented Oct 3, 2023

Unassigning @adhiamboperes since the review is done.

@oppiabot
Copy link

oppiabot bot commented Oct 3, 2023

Hi @aayushimathur6, it looks like some changes were requested on this pull request by @adhiamboperes. PTAL. Thanks!

@aayushimathur6
Copy link
Collaborator Author

PTAL @adhiamboperes Thanks

Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on this @aayushimathur6! LGTM.

@adhiamboperes adhiamboperes enabled auto-merge (squash) October 5, 2023 06:48
@oppiabot
Copy link

oppiabot bot commented Oct 5, 2023

Unassigning @adhiamboperes since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Oct 5, 2023
@oppiabot
Copy link

oppiabot bot commented Oct 5, 2023

Hi @aayushimathur6, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks!

@adhiamboperes adhiamboperes merged commit 3103b0c into oppia:develop Oct 5, 2023
36 checks passed
adhiamboperes pushed a commit that referenced this pull request Dec 5, 2024
<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## Explanation
Fixes [#5168](#5186)
1. For `AppCompatCustomView`, replaced it with AppCompat Views
2. For `FragmentTagUsage`, replaced `<fragment/>` with
<androidx.fragment.app.FragmentContainerView/>. Made some changes to
`NavigationDrawerFragment` to:
- ensured `setUpDrawer()` is called first to initialize the
`drawerLayout`, `toolbar` and `menuItemId`
- followed by `onCreateView()` ensuring to bind the fragment
- followed on `onViewCreated()` to setup drawer functions/listeners
3. Faced an issue similar to [drawer not
working](#5266), by
minimising and resuming the app, (the drawer button stopped working).
- Fixed this by removing `onRestart()` from respective Activies making a
call to `setUpNavigationDrawer()` which at that point is not necessary,
since the fragment has been inflated initially, and would be re-inflated
if the fragment is recreated.
4. Also tested this
[bug](#5284) following the
steps to reproduce. No issues encountered

**Lint report (Before):**
![Screenshot 2024-11-24 at 6 11
50 AM](https://github.com/user-attachments/assets/479da7e9-ea3d-47d5-9c69-ff5c97992750)

**Lint report (After):**
![Screenshot 2024-11-24 at 8 06
27 AM](https://github.com/user-attachments/assets/68ef6cb9-b9b6-45ac-a901-ed64540ee674)

## Screen Record


https://github.com/user-attachments/assets/6db98657-d470-4263-a5ac-d055f4d64e66



## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [x] The PR title and explanation each start with "Fix #bugnum: " (If
this PR fixes part of an issue, prefix the title with "Fix part of
#bugnum: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reformatting required in dimension file for better structure
2 participants