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

[RunAllTests] Fix #3451: [RTL] High-fi Add support for RTL in binding adapters #3452

Merged
merged 120 commits into from
Aug 9, 2021
Merged

[RunAllTests] Fix #3451: [RTL] High-fi Add support for RTL in binding adapters #3452

merged 120 commits into from
Aug 9, 2021

Conversation

veena14cs
Copy link
Contributor

@veena14cs veena14cs commented Jul 9, 2021 •
edited

Explanation

This PR fixes #3451. This PR fixes the custom attributes used in data binding adapters to set margin or padding.
This fix was mainly for content and feedback items of Exploration player.

This PR also fixes #3205.

Screenshot LTR and RTL

Mobile:
screenshot......screenshot

screenshot...... screenshot

Tablet:
screenshot....screenshot

screenshot.....screenshot

Expresso Tests

Screenshot (20)

Screenshot (21)

Screenshot (23)

Screenshots of tests that fails as expected when RTL code is temporarily removed.

Screenshot (32)

Screenshot (30)

Screenshot (34)

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

@veena14cs veena14cs requested a review from rt4914 as a code owner July 9, 2021 19:52
@veena14cs veena14cs added the Type: Task A single task of work corresponding to a greater milestone. Generally corresponds to a single PR. label Jul 9, 2021
@veena14cs veena14cs added this to the RTL Support milestone Jul 9, 2021
@veena14cs veena14cs requested a review from BenHenning July 9, 2021 20:00
@veena14cs
Copy link
Contributor Author

veena14cs commented Jul 9, 2021 •
edited

This PR also fixes #3205. Once this is verified and approved will close PR #3330

Screenshot LTR and RTL

Mobile

screenshot .... screenshot

screenshot ... screenshot

Tablet

screenshot.....screenshot
screenshot.......screenshot

@veena14cs veena14cs mentioned this pull request Jul 9, 2021
Closed
8 tasks
@veena14cs veena14cs changed the title Fix #3451: [RTL] High-fi Exploration content and feedback Fix #3451: [RTL] High-fi Add support for RTL in binding adapters Jul 9, 2021
rt4914
rt4914 suggested changes Jul 10, 2021
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.

@veena14cs PTAL, thanks.

app/src/main/java/org/oppia/android/app/databinding/MarginBindingAdapters.java Show resolved Hide resolved
app/src/main/java/org/oppia/android/app/databinding/MarginBindingAdapters.java Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/android/app/databinding/StateAssemblerMarginBindingAdapters.java Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/android/app/databinding/StateAssemblerPaddingBindingAdapters.java Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/android/app/databinding/MarginBindingAdapters.java Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned veena14cs and unassigned rt4914 Jul 10, 2021
@veena14cs veena14cs requested a review from rt4914 July 10, 2021 18:47
@veena14cs veena14cs removed their assignment Jul 10, 2021
@veena14cs veena14cs removed their assignment Aug 4, 2021
@BenHenning BenHenning assigned veena14cs and unassigned BenHenning Aug 5, 2021
@veena14cs
Copy link
Contributor Author

@BenHenning In that case do I need to go ahead with the changes or revert it?

@veena14cs veena14cs assigned BenHenning and unassigned veena14cs Aug 5, 2021
BenHenning
BenHenning requested changes Aug 6, 2021
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

@veena14cs I think you might not have fully followed the suggestions in my earlier comment. PTAL at the follow-ups.

app/BUILD.bazel Outdated Show resolved Hide resolved
app/BUILD.bazel Outdated Show resolved Hide resolved
app/BUILD.bazel Outdated Show resolved Hide resolved
@BenHenning BenHenning assigned veena14cs and unassigned BenHenning Aug 6, 2021
@veena14cs
Copy link
Contributor Author

@veena14cs I think you might not have fully followed the suggestions in my earlier comment. PTAL at the follow-ups.

I have corrected. PTAL.

anandwana001
anandwana001 approved these changes Aug 6, 2021
View reviewed changes
Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

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

LGTM

@anandwana001 anandwana001 removed their assignment Aug 6, 2021
BenHenning
BenHenning approved these changes Aug 9, 2021
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @veena14cs. This LGTM!

@BenHenning BenHenning merged commit 1405da8 into oppia:develop Aug 9, 2021
@veena14cs veena14cs deleted the fix-exploration-player-texts branch August 9, 2021 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FareesHussain FareesHussain FareesHussain left review comments

@Sparsh1212 Sparsh1212 Sparsh1212 approved these changes

@rt4914 rt4914 rt4914 approved these changes

@BenHenning BenHenning BenHenning approved these changes

@anandwana001 anandwana001 anandwana001 approved these changes

Assignees

@BenHenning BenHenning

@veena14cs veena14cs

Labels
Type: Task A single task of work corresponding to a greater milestone. Generally corresponds to a single PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RTL] High-fi Add support for RTL in binding adapters. [RTL] High-fi Home Screen All Topics List
6 participants
@veena14cs @BenHenning @anandwana001 @Sparsh1212 @rt4914 @FareesHussain