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 #23: Hi fi topic multiple tabs #341

Merged
merged 8 commits into from
Nov 14, 2019

Conversation

nikitamarysolomanpvt
Copy link
Contributor

@nikitamarysolomanpvt nikitamarysolomanpvt commented Nov 13, 2019

The topic page should provide background information on the topic, skills corresponding to the topic (which link to the concept card for that skill), navigation to training on skills in the topic, the progress of stories the user has played in that topic, and links to stories (with preference for in-progress/immediately upcoming stories, though all stories should be viewable). See the PRD for specifics.

Note that this is tracking implementing the final UI for just the topic page. The final UIs for each constituent tab is indicated as a blocker of this issue.

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

Task Items

  • Toolbar/ActionBar title

Screenshot 2019-11-05 at 9 26 38 PM

  • TabLayout with icons and titles

Screenshot 2019-11-05 at 9 26 45 PM

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.

@nikitamarysolomanpvt nikitamarysolomanpvt changed the title Hi fi topic multiple tab Fix part #23: Hi fi topic multiple tabs Nov 13, 2019
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.

Once changes are done, run accessibility scanner to check everything.

Also, one more point: The title of the toolbar is Topic: <TOPIC_NAME>

app/src/main/res/values/styles.xml Outdated Show resolved Hide resolved
app/src/main/res/values/styles.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/topic_fragment.xml Outdated Show resolved Hide resolved
@nikitamarysolomanpvt
Copy link
Contributor Author

nikitamarysolomanpvt commented Nov 13, 2019

@mschanteltc Please find the below issues.
screenshot_Oppia_2019-11-13-20_01_17

  1. Text contrast
    [358,336][451,389]
    The item's text contrast ratio is 4.34. This ratio is based on an estimated foreground color of #B3D1CE and an estimated background color of #00645C. Consider using a contrast ratio greater than 4.50 for small text, or 3.00 for large text.

  2. Text contrast
    [618,336][731,389]
    The item's text contrast ratio is 4.34. This ratio is based on an estimated foreground color of #B3D1CE and an estimated background color of #00645C. Consider using a contrast ratio greater than 4.50 for small text, or 3.00 for large text.

  3. I feel the icons given for tabs are not actually matching the mock. Please provide svg icons 24/ 24 height/width and 24 * 24 viewport

@rt4914
Copy link
Contributor

rt4914 commented Nov 13, 2019

@nikitamarysolomanpvt why have I been assigned again? I am not able to see any changes in this.

@nikitamarysolomanpvt
Copy link
Contributor Author

@nikitamarysolomanpvt why have I been assigned again? I am not able to see any changes in this.

Sorry i merged the code with an old branch which .PTAL now

@rt4914
Copy link
Contributor

rt4914 commented Nov 13, 2019

Once changes are done, run accessibility scanner to check everything.

Also, one more point: The title of the toolbar is Topic: <TOPIC_NAME>

@nikitamarysolomanpvt please make this change.

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.

There is one change remaining, check all the comments.

@rt4914 rt4914 removed their assignment Nov 13, 2019
Copy link
Contributor

@veena14cs veena14cs left a comment

Choose a reason for hiding this comment

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

Looks good please address one comment.

@veena14cs veena14cs removed their assignment Nov 13, 2019
@mschanteltc
Copy link

@mschanteltc Please find the below issues.
screenshot_Oppia_2019-11-13-20_01_17

  1. Text contrast
    [358,336][451,389]
    The item's text contrast ratio is 4.34. This ratio is based on an estimated foreground color of #B3D1CE and an estimated background color of #00645C. Consider using a contrast ratio greater than 4.50 for small text, or 3.00 for large text.
  2. Text contrast
    [618,336][731,389]
    The item's text contrast ratio is 4.34. This ratio is based on an estimated foreground color of #B3D1CE and an estimated background color of #00645C. Consider using a contrast ratio greater than 4.50 for small text, or 3.00 for large text.
  3. I feel the icons given for tabs are not actually matching the mock. Please provide svg icons 24/ 24 height/width and 24 * 24 viewport

For the header text, the font is pure white #FFFFFF while the background is #00645C. This would provide a contrast ratio of 7.05.

For the unselected tab objects, they are all #FFFFFF but have different opacities. The selected that will be 100% opacity. The unselected tabs will increase opacity from 70% to 80%. Using an eyedropper tool, this will make the icons appear as #CCE0DE (do not use this hex for sake of consistency). The contrast this will have against the background would be 5.13.

I will post the icons in the shared Google Drive folder and will let you know when they are available.

@mschanteltc
Copy link

I shared the icons in the Google Drive folder called "Topic Page."

@nikitamarysolomanpvt
Copy link
Contributor Author

I shared the icons in the Google Drive folder called "Topic Page."

Done. But still i feel two of them looks big other two looks smaller comparatively.

@nikitamarysolomanpvt nikitamarysolomanpvt merged commit ea0feb2 into develop Nov 14, 2019
@nikitamarysolomanpvt nikitamarysolomanpvt deleted the hi-fi-topic-multiple-tab branch November 14, 2019 07:18
@nikitamarysolomanpvt nikitamarysolomanpvt restored the hi-fi-topic-multiple-tab branch November 14, 2019 09:46
@nikitamarysolomanpvt nikitamarysolomanpvt deleted the hi-fi-topic-multiple-tab branch November 14, 2019 10:29
@mschanteltc
Copy link

Is it possible you can share a screenshot so I can understand which two needs fixing please?

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.

4 participants