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 #3266: [RTL] High-fi Image from rich text is center aligned in Exploration Player. #3267

Merged
merged 61 commits into from
Aug 5, 2021

Conversation

veena14cs
Copy link
Contributor

@veena14cs veena14cs commented Jun 6, 2021

Explanation

This PR fixes #3266 . This Pr fixes the image alignment to the center rendered from rich text in exploration player.

Document Reference
https://docs.google.com/document/d/1Fl1ar5vcdLvay7ZIJLUFQro1wEf1yUEicwF-CKcvwJ0/edit#heading=h.rfpqdmwhi6tn

Screenshot (LTR and RTL)

Mobile View

screenshot ..... screenshot

screenshot ..... screenshot

Tablet View
screenshot ..... screenshot
screenshot .....screenshot

Screenshot of testcases with RTL code

Screenshot (45)

Screenshot of testcases without RTL code

Screenshot (44)

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 June 6, 2021 18:27
@veena14cs veena14cs changed the title Update content_item.xml Fix #3266: [RTL] High-fi Image from rich text is center aligned in Exploration Player. Jun 6, 2021
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

@@ -68,6 +68,7 @@
android:text="@{htmlContent}"
android:textColor="@color/oppiaPrimaryText"
android:textSize="16sp"
android:textDirection="anyRtl"
Copy link
Contributor

Choose a reason for hiding this comment

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

@veena14cs as per our initial conversation we should not use any attributes like textAlignment, textDirection, etc. I think for this PR it needs to be solved in HTMLParser or related files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed from xml file and added this programmatically, if RTL is enabled.

@rt4914 rt4914 assigned veena14cs and unassigned rt4914 Jun 7, 2021
@veena14cs veena14cs added the Type: Task A single task of work corresponding to a greater milestone. Generally corresponds to a single PR. label Jul 2, 2021
@veena14cs veena14cs added this to the RTL Support milestone Jul 2, 2021
@veena14cs veena14cs requested a review from rt4914 July 14, 2021 12:42
@veena14cs veena14cs assigned rt4914 and BenHenning and unassigned veena14cs Jul 14, 2021
@veena14cs
Copy link
Contributor Author

  1. Could you add a screenshot to the opening comment showing HtmlParserTest passing on Espresso?

There are few old tests that are failing in Expresso. I have also tested this in develop code they failing in develop as well. Do you have any idea on this? Or there is any issue tracking this?

Screenshot (48)

Screenshot (46)

@veena14cs
Copy link
Contributor Author

Thanks @veena14cs! PR generally LGTM, but:

  1. I had one question about the de-flake code you added--PTAL
  2. Could you add a screenshot to the opening comment showing HtmlParserTest passing on Espresso?
  3. Per the label Sparsh added, could you update this to the latest develop?

I have updated code with latest develop.

@veena14cs veena14cs assigned BenHenning and unassigned veena14cs Jul 31, 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! Generally LGTM, but there appears to be dead code to remove.

Regarding the already failing Espresso tests, I don't think that's expected & I'm not aware of a tracking issue. I think we can ignore them for the sake of this PR (it's known that many Espresso tests are in a failing state, and it's hard to resolve them since we can't yet run them in CI).

@BenHenning BenHenning assigned veena14cs and unassigned BenHenning Aug 4, 2021
@veena14cs veena14cs assigned BenHenning and unassigned veena14cs Aug 4, 2021
@veena14cs veena14cs removed the PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged label Aug 4, 2021
@veena14cs
Copy link
Contributor Author

Thanks @veena14cs! Generally LGTM, but there appears to be dead code to remove.

Regarding the already failing Espresso tests, I don't think that's expected & I'm not aware of a tracking issue. I think we can ignore them for the sake of this PR (it's known that many Espresso tests are in a failing state, and it's hard to resolve them since we can't yet run them in CI).

Well in that case I can wrap this PR. I have removed the dead code. Please check.

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.

@BenHenning BenHenning merged commit 1499137 into oppia:develop Aug 5, 2021
@veena14cs veena14cs deleted the rtl-image branch August 9, 2021 09:10
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.

[RTL] High-fi Exploration images to be center aligned
5 participants