Fix #745 : Highfi StoryActivity #758

Scarlet0103 · 2020-03-07T20:26:09Z

Explanation

Fixes : #745
The landscape attributes have been changed according to the mocks given.

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.

Mocks

Portrait Mock: https://xd.adobe.com/view/e8aa4198-3940-47f9-514a-f41cc54457f6-9e9b/screen/5abb0ace-19ee-48b8-8154-919154f924d2/TP-CL-Chapter-List-
Landscape Mock: https://xd.adobe.com/view/ee9e607b-dbd6-4372-48dc-b687d32af3da-98af/screen/cce938d6-e079-4761-bac0-8bde22ac6348/TP-CL-Auto-Scroll-Button-

Screenshots

veena14cs · 2020-03-09T09:03:35Z

@Scarlet0103 Please add screen shots and mock links for reference.

Scarlet0103 · 2020-03-09T09:50:29Z

@Scarlet0103 Please add screen shots and mock links for reference.

Updated PR comment. Thanks...

veena14cs · 2020-03-09T10:01:19Z

@Scarlet0103 Please add screen shots and mock links for reference.

Updated PR comment. Thanks...

Thanks for the screenshot. I found that landscape screenshot is not matching with the mock please check.

Scarlet0103 · 2020-03-09T16:54:10Z

@Scarlet0103 Please add screen shots and mock links for reference.

Updated PR comment. Thanks...

Thanks for the screenshot. I found that landscape screenshot is not matching with the mock please check.

previously the height for the text view of summary was zero. I defined it with min height with the reference from portrait and mocks. There is no text in the viewmodel. @veena14cs

veena14cs · 2020-03-11T06:06:52Z

We use hyphens and not underscores for branch names also names should be in small case. See the example in step 1 here: https://github.com/oppia/oppia-android/wiki#instructions-for-making-a-code-change.

In this Pr its fine. From next PR follow above naming convention.

veena14cs · 2020-03-11T06:18:12Z

previously the height for the text view of summary was zero. I defined it with min height with the reference from portrait and mocks. There is no text in the viewmodel. @veena14cs

If chapter summary is not present then will file an issue and fix it in lowfi. My concern was number of chapters completed text is not aligned with chapters list. Check story_header_view xml file. You need to make changes to all related files. Once you are done asssign me back.

nikitamarysolomanpvt · 2020-03-12T09:13:58Z

@rt4914 Are we supposed to hide image in landscape mode? @Scarlet0103 why the tick icon is not visible?

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

rt4914 · 2020-03-12T12:59:50Z

@rt4914 Are we supposed to hide image in landscape mode? @Scarlet0103 why the tick icon is not visible?

https://xd.adobe.com/view/ee9e607b-dbd6-4372-48dc-b687d32af3da-98af/screen/cce938d6-e079-4761-bac0-8bde22ac6348/TP-CL-Auto-Scroll-Button-/

@nikitamarysolomanpvt Yes image should not be visible in Landscape mode. Also, tick will be visible only when there is progress so this is correct.

@veena14cs @nikitamarysolomanpvt @Scarlet0103
In PR like these where text is used we need make sure that what are the maxlines for any textview.
In this case there are four cases:

Chapter Name - Max Lines - Landscape and Portrait.
Description - Max Lines - Landscape and Portrait

@mschanteltc Can you clarify the number of lines chapter-name and description should take in landscape and portrait mode?

Scarlet0103 · 2020-03-12T20:40:18Z

@veena14cs I generated landcape file for story_header_view and aligned it to the card view. Thanks...

Scarlet0103 · 2020-03-12T21:29:59Z

@veena14cs I generated landcape file for story_header_view and aligned it to the card view. Thanks...

@nikitamarysolomanpvt I made all the summay_text height to wrap content and removed all the min heights defined. so there will be no text cutting. Thanks...

Tested with dummy text

rt4914 · 2020-03-13T10:44:35Z

@Scarlet0103
For landscape mode use min-height of 116 DP.

Also, use following attributes for following cases:

Chapter Name - Max Lines - Landscape (maxLines = 1) and Portrait (maxLines = 2)
Description - Max Lines - Landscape (maxLines = 4) and Portrait (maxLines = 3)

Scarlet0103 · 2020-03-13T10:49:59Z

@rt4914 min height for portrait?

rt4914 · 2020-03-13T10:52:23Z

@rt4914 min height for portrait?

It is already there which is 140 dp

Scarlet0103 · 2020-03-13T11:35:59Z

@rt4914 I made all the changes as you mentioned defined all the min heights and max lines. Also I changed the summary text view height to wrap content from 0dp so that it doesnt give edge cutting text.

Multiline text testing

Singleline Text Testing

rt4914

Changes suggested.

.idea/codeStyles/Project.xml

rt4914 · 2020-03-13T11:49:34Z

.idea/misc.xml

    <output url="file://$PROJECT_DIR$/build/classes" />
  </component>
  <component name="ProjectType">
    <option name="id" value="Android" />
  </component>
-</project>
+</project>


We don't need any change to .idea file, this must have been done automatically from android studio please do not add this file to your commit.
Note: Don't delete this file, we need this file we just don't need the changes this file contains.

@Scarlet0103 this is still pending.

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

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

Scarlet0103 · 2020-03-13T12:07:19Z

Rajat, In the previous mail you gave me these values For landscape mode use min-height of 116 DP. Also, use following attributes for following cases: Chapter Name - Max Lines - Landscape (maxLines = 1) and Portrait (maxLines = 2) Description - Max Lines - Landscape (maxLines = 4) and Portrait (maxLines = 3) *Now should i keep as it is or change them according to your new comment.* *Thanks... *

…

On Fri 13 Mar, 2020, 5:21 PM Rajat Talesra, ***@***.***> wrote: ***@***.**** requested changes on this pull request. Changes suggsted. ------------------------------ In .idea/codeStyles/Project.xml <#758 (comment)>: > @@ -157,4 +159,4 @@ </indentOptions> </codeStyleSettings> </code_scheme> -</component> +</component> We don't need any change to .idea file, this must have been done automatically from android studio please do not add this file to your commit. Note: Don't delete this file, we need this file we just don't need the changes this file contains. ------------------------------ In .idea/misc.xml <#758 (comment)>: > <output url="file://$PROJECT_DIR$/build/classes" /> </component> <component name="ProjectType"> <option name="id" value="Android" /> </component> -</project> +</project> We don't need any change to .idea file, this must have been done automatically from android studio please do not add this file to your commit. Note: Don't delete this file, we need this file we just don't need the changes this file contains. ------------------------------ In app/src/main/res/layout/story_chapter_view.xml <#758 (comment)>: > android:ellipsize="end" android:fontFamily="sans-serif" - android:maxLines="4" + android:maxLines="3" This is incorrect, this file is for portrait mode and as mentioned earlier in portrait mode we have to keep it 4 and in landscape mode we have to keep it 3 ------------------------------ In app/src/main/res/layout-land/story_chapter_view.xml <#758 (comment)>: > android:layout_marginStart="8dp" android:layout_marginTop="8dp" android:layout_marginEnd="8dp" + android:layout_marginBottom="4dp" android:ellipsize="end" android:fontFamily="sans-serif" android:maxLines="4" This should be 3 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#758 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOJPUCDVXK3MRCNI7WZIJ5LRHIM45ANCNFSM4LDSXXVQ> .

rt4914 · 2020-03-13T12:11:10Z

Rajat, In the previous mail you gave me these values For landscape mode use min-height of 116 DP. Also, use following attributes for following cases: Chapter Name - Max Lines - Landscape (maxLines = 1) and Portrait (maxLines = 2) Description - Max Lines - Landscape (maxLines = 4) and Portrait (maxLines = 3) Now should i keep as it is or change them according to your new comment. Thanks... *
…
On Fri 13 Mar, 2020, 5:21 PM Rajat Talesra, @.> wrote: @.** requested changes on this pull request. Changes suggsted. ------------------------------ In .idea/codeStyles/Project.xml <#758 (comment)>: > @@ -157,4 +159,4 @@ </code_scheme> - + We don't need any change to .idea file, this must have been done automatically from android studio please do not add this file to your commit. Note: Don't delete this file, we need this file we just don't need the changes this file contains. ------------------------------ In .idea/misc.xml <#758 (comment)>: > - + We don't need any change to .idea file, this must have been done automatically from android studio please do not add this file to your commit. Note: Don't delete this file, we need this file we just don't need the changes this file contains. ------------------------------ In app/src/main/res/layout/story_chapter_view.xml <#758 (comment)>: > android:ellipsize="end" android:fontFamily="sans-serif" - android:maxLines="4" + android:maxLines="3" This is incorrect, this file is for portrait mode and as mentioned earlier in portrait mode we have to keep it 4 and in landscape mode we have to keep it 3 ------------------------------ In app/src/main/res/layout-land/story_chapter_view.xml <#758 (comment)>: > android:layout_marginStart="8dp" android:layout_marginTop="8dp" android:layout_marginEnd="8dp" + android:layout_marginBottom="4dp" android:ellipsize="end" android:fontFamily="sans-serif" android:maxLines="4" This should be 3 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#758 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOJPUCDVXK3MRCNI7WZIJ5LRHIM45ANCNFSM4LDSXXVQ .

There is typo mistake from my end in landscape and portrait.

Chapter Name - Max Lines - Landscape (maxLines = 1) and Portrait (maxLines = 2)
Description - Max Lines - Landscape (maxLines = 3) and Portrait (maxLines = 4)
Min Height for complete container - Landscape (116dp) and Portrait (140dp)

I hope this clears the confusion.

Scarlet0103 · 2020-03-13T12:24:27Z

@rt4914 Changes Made.Thanks...

rt4914

PTAL (Please take a look) at the comments. Also, before requesting review please make sure that you reply to each and every comment and possibly try to solve it.

.idea/codeStyles/Project.xml

rt4914 · 2020-03-13T12:40:15Z

.idea/misc.xml

    <output url="file://$PROJECT_DIR$/build/classes" />
  </component>
  <component name="ProjectType">
    <option name="id" value="Android" />
  </component>
-</project>
+</project>


@Scarlet0103 this is still pending.

Scarlet0103 · 2020-03-13T12:42:30Z

PTAL (Please take a look) at the comments. Also, before requesting review please make sure that you reply to each and every comment and possibly try to solve it.

sorry my bad.

rt4914

It looks great. There is one small thing that you are missing, in landscape mode if you try to scroll the layout to the very end/bottom, you will see that in mocks the bottom gap is less but in app it is quite big.

To fix this simply change the paddingBottom to 60dp in land/story_fragment recyclerview.

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

…h_fi-Story_Activity

rt4914

LGTM, thanks

rt4914 · 2020-03-13T17:31:34Z

@Scarlet0103 One important thing, in your next PR the branch should follow these two points
all characters should be small and words should be separated by -.

So for example, your current PR name could have been high-fi-story-activity.

mschanteltc · 2020-03-17T00:07:05Z

@rt4914 Are we supposed to hide image in landscape mode? @Scarlet0103 why the tick icon is not visible?

https://xd.adobe.com/view/ee9e607b-dbd6-4372-48dc-b687d32af3da-98af/screen/cce938d6-e079-4761-bac0-8bde22ac6348/TP-CL-Auto-Scroll-Button-/

@nikitamarysolomanpvt Yes image should not be visible in Landscape mode. Also, tick will be visible only when there is progress so this is correct.

@veena14cs @nikitamarysolomanpvt @Scarlet0103
In PR like these where text is used we need make sure that what are the maxlines for any textview.
In this case there are four cases:

Chapter Name - Max Lines - Landscape and Portrait.

Description - Max Lines - Landscape and Portrait

@mschanteltc Can you clarify the number of lines chapter-name and description should take in landscape and portrait mode?

Portrait:

Chapter: 2 lines
Description: 4 lines

Landscape:

Chapter: 2 lines
Description: 3 lines

More info

rt4914 · 2020-03-17T06:08:08Z

@rt4914 Are we supposed to hide image in landscape mode? @Scarlet0103 why the tick icon is not visible?

https://xd.adobe.com/view/ee9e607b-dbd6-4372-48dc-b687d32af3da-98af/screen/cce938d6-e079-4761-bac0-8bde22ac6348/TP-CL-Auto-Scroll-Button-/
@nikitamarysolomanpvt Yes image should not be visible in Landscape mode. Also, tick will be visible only when there is progress so this is correct.
@veena14cs @nikitamarysolomanpvt @Scarlet0103
In PR like these where text is used we need make sure that what are the maxlines for any textview.
In this case there are four cases:

Chapter Name - Max Lines - Landscape and Portrait.

Description - Max Lines - Landscape and Portrait

@mschanteltc Can you clarify the number of lines chapter-name and description should take in landscape and portrait mode?

Portrait:

Chapter: 2 lines

Description: 4 lines

Landscape:

Chapter: 2 lines

Description: 3 lines

More info

Thanks. This is what I thought.

mschanteltc · 2020-03-20T02:15:04Z

@rt4914 Are we supposed to hide image in landscape mode? @Scarlet0103 why the tick icon is not visible?

https://xd.adobe.com/view/ee9e607b-dbd6-4372-48dc-b687d32af3da-98af/screen/cce938d6-e079-4761-bac0-8bde22ac6348/TP-CL-Auto-Scroll-Button-/
@nikitamarysolomanpvt Yes image should not be visible in Landscape mode. Also, tick will be visible only when there is progress so this is correct.
@veena14cs @nikitamarysolomanpvt @Scarlet0103
In PR like these where text is used we need make sure that what are the maxlines for any textview.
In this case there are four cases:

Chapter Name - Max Lines - Landscape and Portrait.

Description - Max Lines - Landscape and Portrait

@mschanteltc Can you clarify the number of lines chapter-name and description should take in landscape and portrait mode?

Portrait:

Chapter: 2 lines

Description: 4 lines

Landscape:

Chapter: 2 lines

Description: 3 lines

More info

Edits on Doc 2:

Portrait:

-Chapter: 2 3 lines
-Description: 4 lines

Landscape:

-Chapter: 2 lines
-Description: 3 lines

Fix #745 : Highfi StoryActivity

0333ffe

rt4914 requested review from nikitamarysolomanpvt and veena14cs March 9, 2020 08:03

rt4914 assigned veena14cs and nikitamarysolomanpvt Mar 9, 2020

veena14cs assigned Scarlet0103 and unassigned veena14cs Mar 9, 2020

rt4914 assigned veena14cs and unassigned Scarlet0103 Mar 10, 2020

veena14cs assigned Scarlet0103 and unassigned veena14cs Mar 11, 2020

nikitamarysolomanpvt reviewed Mar 12, 2020

View reviewed changes

app/src/main/res/layout-land/story_chapter_view.xml Show resolved Hide resolved

nikitamarysolomanpvt removed their assignment Mar 12, 2020

Fix #745 : Highfi StoryActivity

28dfcf9

Fix #745 : Highfi StoryActivity

7258796

Fix #745 : Highfi StoryActivity

61a9956

rt4914 suggested changes Mar 13, 2020

View reviewed changes

NIT changes

491ee57

Scarlet0103 added 2 commits March 13, 2020 17:49

Max lines changed

f98ae15

Max lines changed

514c25e

Scarlet0103 requested a review from rt4914 March 13, 2020 12:26

rt4914 suggested changes Mar 13, 2020

View reviewed changes

Scarlet0103 added 3 commits March 13, 2020 18:21

Update Project.xml

8cd5365

Update gradle.xml

ffb20a8

Update misc.xml

6c03383

Scarlet0103 requested a review from rt4914 March 13, 2020 14:02

rt4914 suggested changes Mar 13, 2020

View reviewed changes

app/src/main/res/layout-land/story_chapter_view.xml Show resolved Hide resolved

Scarlet0103 and others added 3 commits March 13, 2020 22:36

Padding Bottom of land/story_fragment changed

ceaadab

Merge remote-tracking branch 'origin/High_fi-Story_Activity' into Hig…

4a34dfb

…h_fi-Story_Activity

Update Project.xml

c6cfb3d

Scarlet0103 requested a review from rt4914 March 13, 2020 17:24

rt4914 approved these changes Mar 13, 2020

View reviewed changes

rt4914 merged commit 5d8ac56 into oppia:develop Mar 13, 2020

rt4914 mentioned this pull request Mar 17, 2020

Nit change in story_chapter_view #819

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #745 : Highfi StoryActivity #758

Fix #745 : Highfi StoryActivity #758

Scarlet0103 commented Mar 7, 2020 •

edited

Loading

veena14cs commented Mar 9, 2020

Scarlet0103 commented Mar 9, 2020

veena14cs commented Mar 9, 2020

Scarlet0103 commented Mar 9, 2020 •

edited

Loading

veena14cs commented Mar 11, 2020 •

edited

Loading

veena14cs commented Mar 11, 2020 •

edited

Loading

nikitamarysolomanpvt commented Mar 12, 2020

rt4914 commented Mar 12, 2020

Scarlet0103 commented Mar 12, 2020

Scarlet0103 commented Mar 12, 2020

rt4914 commented Mar 13, 2020

Scarlet0103 commented Mar 13, 2020

rt4914 commented Mar 13, 2020

Scarlet0103 commented Mar 13, 2020

rt4914 left a comment •

edited

Loading

rt4914 Mar 13, 2020

rt4914 Mar 13, 2020

Scarlet0103 commented Mar 13, 2020 via email

rt4914 commented Mar 13, 2020

Scarlet0103 commented Mar 13, 2020

rt4914 left a comment

rt4914 Mar 13, 2020

Scarlet0103 commented Mar 13, 2020

rt4914 left a comment

rt4914 left a comment

rt4914 commented Mar 13, 2020

mschanteltc commented Mar 17, 2020

rt4914 commented Mar 17, 2020

mschanteltc commented Mar 20, 2020

Fix #745 : Highfi StoryActivity #758

Fix #745 : Highfi StoryActivity #758

Conversation

Scarlet0103 commented Mar 7, 2020 • edited Loading

Explanation

Checklist

Mocks

Screenshots

veena14cs commented Mar 9, 2020

Scarlet0103 commented Mar 9, 2020

veena14cs commented Mar 9, 2020

Scarlet0103 commented Mar 9, 2020 • edited Loading

veena14cs commented Mar 11, 2020 • edited Loading

veena14cs commented Mar 11, 2020 • edited Loading

nikitamarysolomanpvt commented Mar 12, 2020

rt4914 commented Mar 12, 2020

Scarlet0103 commented Mar 12, 2020

Scarlet0103 commented Mar 12, 2020

rt4914 commented Mar 13, 2020

Scarlet0103 commented Mar 13, 2020

rt4914 commented Mar 13, 2020

Scarlet0103 commented Mar 13, 2020

rt4914 left a comment • edited Loading

Choose a reason for hiding this comment

rt4914 Mar 13, 2020

Choose a reason for hiding this comment

rt4914 Mar 13, 2020

Choose a reason for hiding this comment

Scarlet0103 commented Mar 13, 2020 via email

rt4914 commented Mar 13, 2020

Scarlet0103 commented Mar 13, 2020

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 Mar 13, 2020

Choose a reason for hiding this comment

Scarlet0103 commented Mar 13, 2020

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 commented Mar 13, 2020

mschanteltc commented Mar 17, 2020

rt4914 commented Mar 17, 2020

mschanteltc commented Mar 20, 2020

Scarlet0103 commented Mar 7, 2020 •

edited

Loading

Scarlet0103 commented Mar 9, 2020 •

edited

Loading

veena14cs commented Mar 11, 2020 •

edited

Loading

veena14cs commented Mar 11, 2020 •

edited

Loading

rt4914 left a comment •

edited

Loading