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 #164: Hifi content card #338

Merged
merged 16 commits into from
Nov 18, 2019
Merged

Fix part #164: Hifi content card #338

merged 16 commits into from
Nov 18, 2019

Conversation

veena14cs
Copy link
Contributor

@veena14cs veena14cs commented Nov 13, 2019

Explanation

This Pr corresponds to Content-cards and feedback

Known Issues

  • Explorations data has no image. Need data with image. Resolved

  • There is already padding for some of the text data from JSON file. So its showing too much padding for some of the contents. Resolved here

  • Images are not center aligned.

Mock

https://xd.adobe.com/spec/e2239cf4-9cde-4c08-5296-25316c1f0a14-9412/screen/b7069e13-ae14-44ca-ac1c-c96d43c8f3c4/PM-Q2-CA-Continue-

Screenshot

Screenshot_20191113-181526

GIF-191114_160301

@rt4914
Copy link
Contributor

rt4914 commented Nov 13, 2019

Explanation

This Pr corresponds to Content-cards and feedback

Known Issues

  • Explorations data has no image. Need data with image
  • There is already padding for some of the text data from JSON file. So its showing too much padding for some of the contents.

Mock

https://xd.adobe.com/spec/e2239cf4-9cde-4c08-5296-25316c1f0a14-9412/screen/b7069e13-ae14-44ca-ac1c-c96d43c8f3c4/PM-Q2-CA-Continue-

Screenshot

Screenshot_20191113-181526

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.

@veena14cs please mention these issues in High Fidelity Mock document too.

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, nit changes.

app/src/main/res/layout/feedback_item.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/content_item.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned veena14cs and unassigned rt4914 Nov 13, 2019
@rt4914
Copy link
Contributor

rt4914 commented Nov 13, 2019

@veena14cs I am unassigning @BenHenning and @seanlip from this, considering, this will be merged and then refinalised in Deadline2.

@nikitamarysolomanpvt
Copy link
Contributor

I'am unassigning me please do the nit changes and assign back to me.

@nikitamarysolomanpvt nikitamarysolomanpvt removed their assignment Nov 13, 2019
@veena14cs
Copy link
Contributor Author

veena14cs commented Nov 14, 2019

Resolved Padding issue by using trim() of String.

Screenshot_20191114-160541

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.

Looks really great now.

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

veena14cs commented Nov 15, 2019

Exploration data with image.

Screenshot_20191115-142203

@veena14cs
Copy link
Contributor Author

veena14cs commented Nov 15, 2019

Exploration data with image.

Screenshot_20191115-142203

Issue in this screenshot is bullets. There is new line tag in json data, so the text that has bullets is shown in the new line and with tabs. @BenHenning @rt4914 @seanlip @nikitamarysolomanpvt I need your suggestion on this what we can do for this. For debugging I removed \n\t from json data and here is how it looks.

Screenshot_20191115-142124

@seanlip
Copy link
Member

seanlip commented Nov 15, 2019

Does the JSON data match the JSON data from the server? @vinitamurthi

I think what you've done is fine as a temporary solution, but if the original data matches then we should file an issue to figure out what is going wrong.

Also, the bullets still don't look right. Shouldn't they have spacing around them? I assume they are ul/li elements? I would expect them to look like:

  • Item 1
  • Item 2

@veena14cs
Copy link
Contributor Author

veena14cs commented Nov 15, 2019

Does the JSON data match the JSON data from the server? @vinitamurthi

I think what you've done is fine as a temporary solution, but if the original data matches then we should file an issue to figure out what is going wrong.

Also, the bullets still don't look right. Shouldn't they have spacing around them? I assume they are ul/li elements? I would expect them to look like:

  • Item 1
  • Item 2

Yes you are correct, but when I am removing \n\t I am getting in this form. If I remove only \n it is not making any changes. Below is the data
<ul><li><p><strong>What a "ratio" is</strong></p></li><li><p><strong>How to write ratios using a colon, e.g. “2:3”.</strong></p></li></ul>

@seanlip
Copy link
Member

seanlip commented Nov 15, 2019

Is there a lot of \n\t all over the JSON file? If so then maybe this is an end-of-line encoding issue and we need to strip this stuff out before storing/displaying the JSON. /cc @BenHenning @vinitamurthi

@veena14cs
Copy link
Contributor Author

veena14cs commented Nov 15, 2019 via email

@seanlip
Copy link
Member

seanlip commented Nov 15, 2019

Looks like the \n\t correspond to whitespace in the original HTML file. But generally extra whitespace in HTML should not alter how it's presented? (I'm not sure about the Android quirks though.)

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.

I thought at one point we were replacing \n with
, but I couldn't find that anywhere. It does look like an importing error @vinitamurthi. Any ideas?

@BenHenning BenHenning removed their assignment Nov 15, 2019
@BenHenning
Copy link
Member

Let's submit this without the proper HTML parsing--I filed #361 to tack that work.

@BenHenning
Copy link
Member

This should also fix #372.

@veena14cs
Copy link
Contributor Author

Let's submit this without the proper HTML parsing--I filed #361 to tack that work.

Okay I will create this work in seperate PR.

@veena14cs
Copy link
Contributor Author

#361

Here is the PR for this fix #404

@veena14cs veena14cs merged commit 1f0e828 into develop Nov 18, 2019
@veena14cs veena14cs deleted the hifi-content-card branch November 18, 2019 09:01
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.

7 participants