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

Fix #3780: [RTL] Image region selection. #3815

Merged
merged 88 commits into from
Sep 27, 2021

Conversation

veena14cs
Copy link
Contributor

@veena14cs veena14cs commented Sep 18, 2021

Explanation

Fix #3780. This PR fixes Image region selection issues in RTL layouts when tapping around the image.

Screenshot LTR and RTL

Mobile:
Screenshot_1632187231........Screenshot_1632187141

Screenshot_1632187215.......Screenshot_1632187192

Screenshot for Tests that pass in Expresso after RTL code.

Screenshot (11)

Screenshot for Tests that fail in Expresso before RTL code.

Screenshot (12)

Note: Not tested on Roboelectric added a todo comment in the test.

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

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.

LGTM.

For tests I will defer to @anandwana001 Thanks.

@rt4914 rt4914 removed their assignment Sep 24, 2021
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! LGTM though I think we can & should re-enable the tests. See my comment.

@@ -280,6 +281,68 @@ class ImageRegionSelectionInteractionViewTest {
}
}

@Test
// TODO(#1611): Fix ImageRegionSelectionInteractionViewTest
@Ignore
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Actually, given this can we use OppiaTestRule and RunOn(TestPlatform.ESPRESSO) here instead of ignoring? We don't actually need to ignore tests failing for specific platforms anymore.

@BenHenning BenHenning removed their assignment Sep 26, 2021
@veena14cs
Copy link
Contributor Author

veena14cs commented Sep 26, 2021

Thanks @veena14cs! LGTM though I think we can & should re-enable the tests. See my comment.

Thanks @BenHenning I have re-enabled tests and updated screenshots in the PR description.
One test was ignored that is to be moved to Roboelectric as per the message.

@@ -118,7 +121,7 @@ class ImageRegionSelectionInteractionViewTest {

Copy link
Contributor

Choose a reason for hiding this comment

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

You had to set the oppia test rule, else the RunOn annotation will not work.

  @get:Rule
  val oppiaTestRule = OppiaTestRule()

Copy link
Contributor

Choose a reason for hiding this comment

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

HelpFragmentTest for reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested it without this Rule and I I wonder how it worked. Added it now it shouldn't break anywhere. Thanks for reference.

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.

LGTM from code perspective other than the issue @anandwana001 mentioned.

@anandwana001 deferring to you to verify CI is fully passing before merging.

Thanks for quick fix @veena14cs.

@BenHenning BenHenning removed their assignment Sep 27, 2021
@veena14cs
Copy link
Contributor Author

LGTM from code perspective other than the issue @anandwana001 mentioned.

@anandwana001 deferring to you to verify CI is fully passing before merging.

Thanks for quick fix @veena14cs.

Thanks @BenHenning.

@anandwana001
Copy link
Contributor

Assign me once finalize. LGTM for merge.

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

@veena14cs veena14cs assigned anandwana001 and unassigned veena14cs Sep 27, 2021
@anandwana001 anandwana001 merged commit 713584a into oppia:develop Sep 27, 2021
@veena14cs
Copy link
Contributor Author

Assign me once finalize. LGTM for merge.

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Task A single task of work corresponding to a greater milestone. Generally corresponds to a single PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image region selection doesn't work in RTL layouts
5 participants