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.

Sign up for GitHub

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

Jump to bottom

Fix part #141 : Final UI for topic thumbnail (Image) with gradient over image (without topic-name). #291

Merged
merged 22 commits into from
Nov 14, 2019
Merged

Fix part #141 : Final UI for topic thumbnail (Image) with gradient over image (without topic-name). #291

merged 22 commits into from
Nov 14, 2019

Conversation

veena14cs
Copy link
Contributor

@veena14cs veena14cs commented Oct 31, 2019 •
edited
Loading

Explaination

This PR contain part of High-fi design topic thumbnail (Image) with gradient over image inside topic-overview tab fragment.

Accessibility Scanner

  • Accessibility Scanner is working correctly

Mock

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

Screenshot

Below screenshot is updated with this image #291 (comment)

Screenshot_20191107-131440

@veena14cs veena14cs requested a review from rt4914 October 31, 2019 18:54
@veena14cs veena14cs changed the title Fix part #141 : Final UI for Topic overview tab's topic thumbnail (Image) with gradient over image. Fix part #141 : Final UI for topic thumbnail (Image) with gradient over image (without topic-name). Oct 31, 2019
rt4914
rt4914 suggested changes Nov 4, 2019
View reviewed changes
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.

Generally LGTM, but do make these nit changes and re-assign it to me.

app/src/main/res/layout/topic_overview_fragment.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/topic_overview_fragment.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/topic_overview_fragment.xml Outdated Show resolved Hide resolved
app/src/main/res/drawable/thumbnail_gradient.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned veena14cs and unassigned rt4914 Nov 4, 2019
nikitamarysolomanpvt
nikitamarysolomanpvt reviewed Nov 4, 2019
View reviewed changes
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 please make the nit changes.

app/src/main/res/layout/topic_overview_fragment.xml Outdated Show resolved Hide resolved
@nikitamarysolomanpvt nikitamarysolomanpvt removed their assignment Nov 4, 2019
@veena14cs veena14cs requested a review from rt4914 November 7, 2019 06:17
@veena14cs veena14cs assigned rt4914 and unassigned veena14cs Nov 7, 2019
rt4914
rt4914 approved these changes Nov 7, 2019
View reviewed changes
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. Suggest assigning this PR to @seanlip and @BenHenning for final review.

app/src/main/res/layout/topic_overview_fragment.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/topic_overview_fragment.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 removed their assignment Nov 7, 2019
@rt4914 rt4914 removed their assignment Nov 8, 2019
nikitamarysolomanpvt
nikitamarysolomanpvt approved these changes Nov 8, 2019
View reviewed changes
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

@nikitamarysolomanpvt nikitamarysolomanpvt removed their assignment Nov 8, 2019
@BenHenning
Copy link
Member

Is there a way we can do this without losing the image fidelity of using vector graphics? Once concern I have with using PNGs is they won't scale well for larger screens when we support them (such as tablets).

BenHenning
BenHenning requested changes Nov 12, 2019
View reviewed changes
Copy link
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.

Requesting changes per my earlier comment.

@BenHenning BenHenning removed their assignment Nov 12, 2019
@veena14cs
Copy link
Contributor Author

veena14cs commented Nov 12, 2019 •
edited
Loading

Is there a way we can do this without losing the image fidelity of using vector graphics? Once concern I have with using PNGs is they won't scale well for larger screens when we support them (such as tablets).

I have used vector image below is the screenshot. But this image has got no padding around it. @mschanteltc could you please provide vector image for this. Also many vector images have no padding around them and its touching boundaries.

Screenshot_20191112-153336

@veena14cs veena14cs requested a review from rt4914 November 12, 2019 10:15
@veena14cs veena14cs assigned rt4914 and unassigned veena14cs Nov 12, 2019
rt4914
rt4914 suggested changes Nov 12, 2019
View reviewed changes
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.

Please check comments.

app/src/main/res/drawable/rectangle_background.xml Show resolved Hide resolved
app/src/main/java/org/oppia/app/databinding/DrawableBindingAdapters.kt Show resolved Hide resolved
@rt4914 rt4914 assigned veena14cs and unassigned rt4914 Nov 12, 2019
@veena14cs veena14cs assigned rt4914 and unassigned veena14cs Nov 12, 2019
@mschanteltc
Copy link

Is there a way we can do this without losing the image fidelity of using vector graphics? Once concern I have with using PNGs is they won't scale well for larger screens when we support them (such as tablets).

I have used vector image below is the screenshot. But this image has got no padding around it. @mschanteltc could you please provide vector image for this. Also many vector images have no padding around them and its touching boundaries.

Screenshot_20191112-153336

I created two SVG files: one with a blue background and one with a transparent background. Both have 'padding' around the left, right, and top of the object. I sent them in an email because Github does not support uploading SVG files.

rt4914
rt4914 approved these changes Nov 13, 2019
View reviewed changes
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

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

I have added vector image provided by @mschanteltc.

Screenshot_20191113-114615

BenHenning
BenHenning approved these changes Nov 13, 2019
View reviewed changes
Copy link
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 for switching back to vector graphics @veena14cs! This LGTM.

@veena14cs
Copy link
Contributor Author

Thanks for switching back to vector graphics @veena14cs! This LGTM.

Thanks.

@veena14cs veena14cs merged commit c1d1798 into develop Nov 14, 2019
@veena14cs veena14cs deleted the hifi-topic-thumbnail branch November 14, 2019 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mschanteltc mschanteltc mschanteltc left review comments

@nikitamarysolomanpvt nikitamarysolomanpvt nikitamarysolomanpvt approved these changes

@BenHenning BenHenning BenHenning approved these changes

@rt4914 rt4914 rt4914 approved these changes

@seanlip seanlip Awaiting requested review from seanlip

Assignees

@veena14cs veena14cs

@mschanteltc mschanteltc

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@veena14cs @seanlip @mschanteltc @BenHenning @rt4914 @nikitamarysolomanpvt