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.

Sign up for GitHub

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

Jump to bottom

Fix: #3411 Merge administrator_controls_download_permissions_view into single xml file. #4175

Merged
merged 6 commits into from
Feb 21, 2022
Merged

Fix: #3411 Merge administrator_controls_download_permissions_view into single xml file. #4175

merged 6 commits into from
Feb 21, 2022

Conversation

Akshatkamboj14
Copy link
Member

@Akshatkamboj14 Akshatkamboj14 commented Feb 13, 2022 •
edited

Explanation

FIXES #3411

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
BEFORE AFTER
Screenshot_20220213_000010 Screenshot_20220215_124149
Screenshot_20220215_122455 Screenshot_20220215_124233
Screenshot_20220213_001428 Screenshot_20220213_111012
Screenshot_20220213_114014 Screenshot_20220209_213851

@Akshatkamboj14
Copy link
Member Author

@rishidyno please review it

rishidyno
rishidyno suggested changes Feb 13, 2022
View reviewed changes
app/src/main/res/values-sw600dp-land/dimens.xml Outdated
<dimen name="administrator_controls_download_permissions_view_auto_update_topic_constraint_layout_padding_start">16dp</dimen>
<dimen name="administrator_controls_download_permissions_view_auto_update_topic_constraint_layout_padding_end">16dp</dimen>
<dimen name="administrator_controls_download_permissions_view_auto_update_topic_widget_switch_compat_margin_end">4dp</dimen>

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this extra line.

app/src/main/res/values-sw600dp-port/dimens.xml Outdated
<dimen name="administrator_controls_download_permissions_view_auto_update_topic_constraint_layout_padding_start">16dp</dimen>
<dimen name="administrator_controls_download_permissions_view_auto_update_topic_constraint_layout_padding_end">16dp</dimen>
<dimen name="administrator_controls_download_permissions_view_auto_update_topic_widget_switch_compat_margin_end">4dp</dimen>

Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@oppiabot
Copy link

oppiabot bot commented Feb 13, 2022

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

@rishidyno
Copy link
Contributor

And try to fix the failing check. I think this should not happen but seems to happen in other ps as well. Just commit the requested changes and will see what happens.

@rishidyno
Copy link
Contributor

Also update your project.

@Akshatkamboj14
Copy link
Member Author

done @rishidyno

@rishidyno rishidyno closed this Feb 14, 2022
@rishidyno rishidyno reopened this Feb 14, 2022
@rt4914 rt4914 assigned rishidyno and unassigned Akshatkamboj14 Feb 14, 2022
rishidyno
rishidyno suggested changes Feb 14, 2022
View reviewed changes
Copy link
Contributor

@rishidyno rishidyno left a comment •
edited

Choose a reason for hiding this comment

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

@Akshatkamboj14 I think this pr is good to go, but always use same device to take SS. I suggest updating SS for mobile-land. So that we can be sure that there are no changes in UI before and after. Also, use an emulator without round corners. Thanks.

@oppiabot
Copy link

oppiabot bot commented Feb 14, 2022

Unassigning @rishidyno since the review is done.

@oppiabot
Copy link

oppiabot bot commented Feb 14, 2022

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

@Akshatkamboj14
Copy link
Member Author

hey, @rishidyno I have updated the ss of mobile ones with the new emulator.

@rishidyno
Copy link
Contributor

@Akshatkamboj14 also resolve the merge conflict

@rishidyno
Copy link
Contributor

Also please keep the Assignees section up-to-date, so that we can know whether to review this PR or not.

@rishidyno
Copy link
Contributor

rishidyno commented Feb 15, 2022 •
edited

FYI the person who has to review should be assigned

rishidyno
rishidyno suggested changes Feb 15, 2022
View reviewed changes
app/src/main/res/values-sw600dp-land/dimens.xml Outdated
@@ -419,8 +419,18 @@
<dimen name="revision_card_fragment_padding_bottom">148dp</dimen>
<dimen name="revision_card_fragment_layout_text_margin_top">32dp</dimen>
<dimen name="revision_card_fragment_layout_button_margin_top">24dp</dimen>

Copy link
Contributor

Choose a reason for hiding this comment

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

PTAL --> this can be corrected.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@rishidyno
Copy link
Contributor

LGTM thanks @Akshatkamboj14 .

@rishidyno rishidyno assigned rt4914 and rishidyno and unassigned Akshatkamboj14 and rt4914 Feb 15, 2022
@rt4914 rt4914 assigned rt4914 and unassigned rishidyno Feb 15, 2022
rt4914
rt4914 approved these changes Feb 21, 2022
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

This is really clean code. Great work @Akshatkamboj14 and @rishidyno

LGTM

@rt4914 rt4914 merged commit bb4ad2e into oppia:develop Feb 21, 2022
@Akshatkamboj14 Akshatkamboj14 self-assigned this Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rishidyno rishidyno rishidyno requested changes

@rt4914 rt4914 rt4914 approved these changes

Assignees

@rt4914 rt4914

@Akshatkamboj14 Akshatkamboj14

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Merge administrator_controls_download_permissions_view into single xml file.
3 participants
@Akshatkamboj14 @rishidyno @rt4914