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 part #141: Hi-Fi Topic overview tab fragment #342

Merged
merged 9 commits into from
Nov 22, 2019

Conversation

veena14cs
Copy link
Contributor

@veena14cs veena14cs commented Nov 14, 2019

Explanation

This PR corresponds to final UI for Topic Overview tab

Mock
https://xd.adobe.com/spec/e2239cf4-9cde-4c08-5296-25316c1f0a14-9412/screen/19cfbacf-854c-4c7d-8691-3b3d117e1866/TP-Overview-/

Screenshot

image

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, just use shortcut Option+cmd+L to reorganise.

@rt4914 rt4914 assigned veena14cs and unassigned rt4914 Nov 14, 2019
Copy link
Contributor

@nikitamarysolomanpvt nikitamarysolomanpvt left a comment

Choose a reason for hiding this comment

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

LGTM.
@mschanteltc
PTAL on the below accessibility test.
screenshot_Oppia_2019-11-14-15_47_07.
Item descriptions
[40,96][449,163]
This non-clickable item's speakable text: "Second Test Topic" is identical to that of 1 other item(s).

If we remove the topic name from topic review screen this would get resolved ... Please suggest ...

@mschanteltc
Copy link

Can you clarify what this means? What is not passing the Accessibility Checker?

[40,96][449,163]

We need the heading so users know what Topic they are viewing.

@nikitamarysolomanpvt
Copy link
Contributor

nikitamarysolomanpvt commented Nov 19, 2019

Can you clarify what this means? What is not passing the Accessibility Checker?

[40,96][449,163]

We need the heading so users know what Topic they are viewing.

@mschanteltc
Topic title (Second Test Topic) is repeated twice in the same screen so was suggested in the accessibility scanner to avoid this.

@nikitamarysolomanpvt nikitamarysolomanpvt removed their assignment Nov 19, 2019
@mschanteltc
Copy link

@seanlip and I discussed this issue and despite the Topic name repeated twice, we should keep the design as it is. It's a minor issue and the title of the thumbnail gives clarity to the details and descriptions of the Lesson.

@mschanteltc mschanteltc assigned veena14cs and seanlip and unassigned mschanteltc Nov 21, 2019
@seanlip
Copy link
Member

seanlip commented Nov 22, 2019

@veena14cs and @nikitamarysolomanpvt, I have a question about this. I looked up the message and it seems like this is an "info" message, not an "error" / "warning". Could you please confirm this?

https://github.com/google/Accessibility-Test-Framework-for-Android/blob/master/src/main/java/com/google/android/apps/common/testing/accessibility/framework/checks/DuplicateSpeakableTextCheck.java#L41

If it's an info message, we can leave it as is. If it's an error/warning, this needs further discussion.

Thanks!

@veena14cs
Copy link
Contributor Author

@veena14cs and @nikitamarysolomanpvt, I have a question about this. I looked up the message and it seems like this is an "info" message, not an "error" / "warning". Could you please confirm this?

https://github.com/google/Accessibility-Test-Framework-for-Android/blob/master/src/main/java/com/google/android/apps/common/testing/accessibility/framework/checks/DuplicateSpeakableTextCheck.java#L41

If it's an info message, we can leave it as is. If it's an error/warning, this needs further discussion.

Thanks!

It was just a suggestion from scanner that, multiple items have same description. So we can ignore this. Thanks.

@veena14cs veena14cs merged commit 078afb1 into develop Nov 22, 2019
@veena14cs veena14cs deleted the hifi-topic-overview branch November 22, 2019 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants