Fix #2093: [Mobile-Landscape] High-fi StoryActivity Updated UI #2685

anshbajpai · 2021-02-11T09:29:08Z

Explanation

Fixes #2093
Updated, landscape UI according to the given changes.
Updated Screenshots:

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.

anshbajpai · 2021-02-11T10:16:56Z

@FareesHussain are there any changes, I should make?

FareesHussain

@anshbajpai Add few comments please check
Update the PR description add Fixes #2093

FareesHussain · 2021-02-11T10:07:04Z

app/src/main/res/layout-land/story_chapter_view.xml

- android:fontFamily="sans-serif-medium"
+ android:fontFamily="sans-serif"


Is this change according to the mock.

no, the mock uses Roboto font, but I used sans-serif as it was being used earlier. Can I change it to Roboto?

FareesHussain · 2021-02-11T10:09:20Z

app/src/main/res/layout-land/story_chapter_view.xml

 android:maxLines="2"
- android:text="@{String.format(@string/chapter_name, (viewModel.index + 1), viewModel.name)}"
+ android:text="@{titleContent}"


Why are you updating the title here. The issue is only related to the borders right.

yeah, but also if some pre-requisites are not completed, the title has to be changed to - "to unlock this chapter, you have to finish this chapter", hence I used the title here

FareesHussain · 2021-02-11T10:17:44Z

app/src/main/java/org/oppia/android/app/story/StoryFragmentPresenter.kt

+ str,
+ binding.chapterTitle
+ )
+


Remove this extra line. This is causing the lint test error.
To avoid this I suggest to setup the ktlint pre-push for the project

prayutsu · 2021-02-11T12:06:50Z

app/src/main/java/org/oppia/android/app/story/StoryFragmentPresenter.kt

+ val str = if (storyItemViewModel.chapterSummary.chapterPlayState ==
+ ChapterPlayState.NOT_PLAYABLE_MISSING_PREREQUISITES
+ ) {
+ String.format(
+ HtmlCompat.toHtml(
+ SpannedString
+ .valueOf(view.context.getText(R.string.unlock_chapter)),
+ HtmlCompat.TO_HTML_PARAGRAPH_LINES_CONSECUTIVE
+ ),
+ storyItemViewModel.index,
+ storyItemViewModel.missingPrerequisiteChapter.name
+ )
+ } else {
+ String.format(
+ HtmlCompat.toHtml(
+ SpannedString.valueOf(
+ view.context
+ .getText(R.string.chapter_name)
+ ),
+ HtmlCompat.TO_HTML_PARAGRAPH_LINES_CONSECUTIVE
+ ),
+ storyItemViewModel.index + 1,
+ storyItemViewModel.name
+ )
+ }
+ binding.storyChapterCard.strokeWidth =
+ when (storyItemViewModel.chapterSummary.chapterPlayState) {
+ ChapterPlayState.NOT_PLAYABLE_MISSING_PREREQUISITES ->
+ view.context.resources.getDimension(R.dimen.space_0dp).toInt()
+ else ->
+ view.context.resources.getDimension(R.dimen.story_chapter_stroke_width).toInt()
+ }
+ binding.titleContent =
+ htmlParserFactory.create(
+ resourceBucketName,
+ entityType,
+ storyItemViewModel.storyId,
+ imageCenterAlign = true
+ ).parseOppiaHtml(
+ str,
+ binding.chapterTitle
+ )


I believe this can be done with Data binding only, we want to abstract away this as much as possible, so try doing it with data binding only.

anshbajpai · 2021-02-11T15:24:19Z

@prayutsu I have updated the code and now it's only using data binding

rt4914

There are some issues with the mocks. Adding @mschanteltc for clarifications:

Portrait
Landscape

In the locked chapter the portrait mock contains Chapter name and landscape mode does not which is inconsistent especially for A11y cases. So I think we should add Chapter name title in both cases.
In the statement "Complete Chapter 2: The Meaning of “Equal Parts” to unlock this chapter." we are using two types of fonts: Regular and Medium. Is this possible to update this to Regular and Bold so that it because easy on android side to implement this.

@mschanteltc WDYT about above points?

rt4914 · 2021-02-11T15:47:33Z

app/src/main/res/layout-land/story_chapter_view.xml

+<!-- android:layout_marginBottom="8dp"-->
+<!-- android:background="@color/chapterCardShadow"-->
+<!-- android:padding="8dp"-->
+<!-- android:visibility="@{viewModel.chapterSummary.chapterPlayState == ChapterPlayState.NOT_PLAYABLE_MISSING_PREREQUISITES ? View.VISIBLE : View.INVISIBLE}" />-->


Remove these comments.

rt4914 · 2021-02-11T15:47:50Z

app/src/main/res/values/strings.xml

@@ -64,6 +64,7 @@
 <string name="chapter_name">Chapter %s: %s</string>
 <string name="chapter_completed">Chapter %s: %s is completed.</string>
 <string name="chapter_prerequisite_title_label">Complete Chapter %s: %s to unlock this chapter.</string>
+ <string name="empty_string"> </string>


This is not correct. We should not use empty_strings.

rt4914 · 2021-02-11T15:53:47Z

app/src/main/res/values/strings.xml

@@ -422,4 +423,6 @@
 </string>
 <string name="smallest_text_size_content_description">Smallest text size</string>
 <string name="largest_text_size_content_description">Largest text size</string>
+ <string name="unlock_chapter">"Complete <![CDATA[<b>Chapter %s: %s</b>]]> to unlock this chapter."</string>
+ <string name="chapter_land_name"><![CDATA[<b>Chapter %s: %s</b>]]></string>


This is incorrect. Making the text bold is not same as Roboto-Medium.

@rt4914 I agree, but I was not sure whether I can use Roboto-Medium or not, as it's an external font

This solution will work for now.

mschanteltc · 2021-02-13T07:38:05Z

There are some issues with the mocks. Adding @mschanteltc for clarifications:

Portrait
Landscape

In the locked chapter the portrait mock contains Chapter name and landscape mode does not which is inconsistent especially for A11y cases. So I think we should add Chapter name title in both cases.

In the statement "Complete Chapter 2: The Meaning of “Equal Parts” to unlock this chapter." we are using two types of fonts: Regular and Medium. Is this possible to update this to Regular and Bold so that it because easy on android side to implement this.

@mschanteltc WDYT about above points?

Yes, I updated the mobile and tablet mocks for both portrait and landscape orientations so that each one includes the Chapter name.
If we bold the name, can we add a feature where if the user taps on the prerequisite Chapter Name, the page auto scrolls to that respective Chapter card? If so, can we also make the Chapter Name a different color so that it's distinguishable and can be recognized as a button? I edited the mobile portrait mock so you can see how the feature works. (Scroll to 'Chapter 3: Fractions of a Group' card and tap 'Chapter 2: The Meaning of "Equal Parts"')

anshbajpai · 2021-02-13T13:44:30Z

@mschanteltc @rt4914 should I change the font to Roboto for the landscape version according to the mock, if yes - should I add the font as a downloadable font or simply add it in the project ?

rt4914 · 2021-02-15T14:37:59Z

There are some issues with the mocks. Adding @mschanteltc for clarifications:
Portrait
Landscape

In the locked chapter the portrait mock contains Chapter name and landscape mode does not which is inconsistent especially for A11y cases. So I think we should add Chapter name title in both cases.

In the statement "Complete Chapter 2: The Meaning of “Equal Parts” to unlock this chapter." we are using two types of fonts: Regular and Medium. Is this possible to update this to Regular and Bold so that it because easy on android side to implement this.

@mschanteltc WDYT about above points?

Yes, I updated the mobile and tablet mocks for both portrait and landscape orientations so that each one includes the Chapter name.

If we bold the name, can we add a feature where if the user taps on the prerequisite Chapter Name, the page auto scrolls to that respective Chapter card? If so, can we also make the Chapter Name a different color so that it's distinguishable and can be recognized as a button? I edited the mobile portrait mock so you can see how the feature works. (Scroll to 'Chapter 3: Fractions of a Group' card and tap 'Chapter 2: The Meaning of "Equal Parts"')

@mschanteltc We can do that but the technical cost might be too much which might not be needed for this feature. Adding @BenHenning for his input. @BenHenning Do you think we should apply what Chantel has suggested?

BenHenning · 2021-02-17T05:49:36Z

@rt4914 I think that might be a nice stretch project, but shouldn't be a hard blocker. It can be implemented similarly to how we implement non-interactive links elsewhere in the app (e.g. using a custom tag). It seems like a nice improvement.

rt4914 · 2021-02-23T14:39:56Z

@anshbajpai Have a look at this mock: https://xd.adobe.com/view/e8aa4198-3940-47f9-514a-f41cc54457f6-9e9b/screen/5abb0ace-19ee-48b8-8154-919154f924d2/

As per the third card in this. The Chapter-2 should be clickable. For now we can keep it non-clickable and lets use regular and bold so that it looks like this but in black color. Similar to what you did in current solution.

FareesHussain · 2021-03-01T18:55:08Z

@anshbajpai any updates on this?

anshbajpai · 2021-03-02T04:16:02Z

@anshbajpai any updates on this?

yeah , i am working on it, will update the pr in 1 - 2 days probably

FareesHussain · 2021-03-07T15:50:32Z

@anshbajpai Any updates on this?

rt4914 · 2021-03-15T12:30:57Z

Closing this because of no updates in 2 weeks.

Fix oppia#2093: [Mobile-Landscape] High-fi StoryActivity Updated UI

d9068d7

anshbajpai requested a review from rt4914 as a code owner February 11, 2021 09:29

Fix oppia#2093 Code Formatting Improved

3a34b0d

FareesHussain self-assigned this Feb 11, 2021

FareesHussain suggested changes Feb 11, 2021

View reviewed changes

rt4914 self-assigned this Feb 11, 2021

FareesHussain removed their assignment Feb 11, 2021

Fix oppia#2093 - Linting issue fixes

67fc43e

prayutsu suggested changes Feb 11, 2021

View reviewed changes

Fix oppia#2093 Shifted Code, using databinding only

61ec1c8

rt4914 suggested changes Feb 11, 2021

View reviewed changes

rt4914 assigned mschanteltc and unassigned rt4914 Feb 11, 2021

Fix oppia#2093 - Removed empty string and unnecessary comments

25a5473

= and others added 5 commits February 13, 2021 19:24

Fix oppia#2093 - Updated UI

745028c

Fix oppia#2093 - Updated UI

d71e646

Fix oppia#2093 - Resolved Conflict

f74c509

Fix oppia#2093 - Changed strings.xml

a03a31a

Merge branch 'develop' into updated_ui

a510fe3

anshbajpai requested a review from rt4914 February 15, 2021 11:42

rt4914 assigned BenHenning Feb 15, 2021

BenHenning assigned rt4914 and unassigned BenHenning Feb 17, 2021

rt4914 assigned anshbajpai and unassigned rt4914 and mschanteltc Feb 23, 2021

BenHenning added 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 Feb 24, 2021

Merge branch 'develop' into updated_ui

8acbb67

BenHenning 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 Feb 24, 2021

rt4914 closed this Mar 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #2093: [Mobile-Landscape] High-fi StoryActivity Updated UI #2685

Fix #2093: [Mobile-Landscape] High-fi StoryActivity Updated UI #2685

anshbajpai commented Feb 11, 2021 •

edited

Loading

anshbajpai commented Feb 11, 2021

FareesHussain left a comment

FareesHussain Feb 11, 2021

anshbajpai Feb 11, 2021

FareesHussain Feb 11, 2021

anshbajpai Feb 11, 2021

FareesHussain Feb 11, 2021

anshbajpai Feb 11, 2021

prayutsu Feb 11, 2021

anshbajpai Feb 11, 2021

anshbajpai commented Feb 11, 2021

rt4914 left a comment

rt4914 Feb 11, 2021

rt4914 Feb 11, 2021

rt4914 Feb 11, 2021

anshbajpai Feb 11, 2021

rt4914 Feb 23, 2021

mschanteltc commented Feb 13, 2021

anshbajpai commented Feb 13, 2021

rt4914 commented Feb 15, 2021

BenHenning commented Feb 17, 2021 •

edited

Loading

rt4914 commented Feb 23, 2021

FareesHussain commented Mar 1, 2021

anshbajpai commented Mar 2, 2021

FareesHussain commented Mar 7, 2021

rt4914 commented Mar 15, 2021

		android:fontFamily="sans-serif-medium"
		android:fontFamily="sans-serif"

Fix #2093: [Mobile-Landscape] High-fi StoryActivity Updated UI #2685

Fix #2093: [Mobile-Landscape] High-fi StoryActivity Updated UI #2685

Conversation

anshbajpai commented Feb 11, 2021 • edited Loading

Explanation

Checklist

anshbajpai commented Feb 11, 2021

FareesHussain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshbajpai commented Feb 11, 2021

rt4914 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mschanteltc commented Feb 13, 2021

anshbajpai commented Feb 13, 2021

rt4914 commented Feb 15, 2021

BenHenning commented Feb 17, 2021 • edited Loading

rt4914 commented Feb 23, 2021

FareesHussain commented Mar 1, 2021

anshbajpai commented Mar 2, 2021

FareesHussain commented Mar 7, 2021

rt4914 commented Mar 15, 2021

anshbajpai commented Feb 11, 2021 •

edited

Loading

BenHenning commented Feb 17, 2021 •

edited

Loading