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

#145 Product Page: Learners Carousel #237

Merged

Conversation

ahmed-arbisoft
Copy link
Contributor

@ahmed-arbisoft ahmed-arbisoft commented May 8, 2019

Pre-Flight checklist

  • Screenshots and design review for any changes that affect layout or styling
    • Desktop screenshots
    • Mobile width screenshots
    • Tag @Ferdi or @pdpinch for review
  • Migrations
    • Migration is backwards-compatible with current production code
  • Testing
    • Code is tested
    • Changes have been manually tested

What are the relevant tickets?

Closes #145

What's this PR do?

Adds a UserTestimonialsPage type subpage as candidate child to a product page.

How should this be manually tested?

Add the respective subpage to a product page, the carousel should appear as per specs, showing testimonials added via the wagtail admin.

Screenshots (if appropriate)

Screenshot from 2019-05-09 01-21-39
Screenshot from 2019-05-09 16-30-47
Screenshot from 2019-05-09 01-28-25
Screenshot from 2019-05-09 16-29-06

@odlbot odlbot temporarily deployed to xpro-ci-pr-237 May 8, 2019 20:30 Inactive
@codecov-io
Copy link

codecov-io commented May 8, 2019

Codecov Report

Merging #237 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #237      +/-   ##
=========================================
+ Coverage   94.27%   94.3%   +0.02%     
=========================================
  Files         157     157              
  Lines        5133    5160      +27     
  Branches      309     309              
=========================================
+ Hits         4839    4866      +27     
  Misses        238     238              
  Partials       56      56
Impacted Files Coverage Δ
courses/models.py 95.88% <100%> (+0.04%) ⬆️
cms/blocks.py 100% <100%> (ø) ⬆️
cms/models.py 93.75% <100%> (+0.41%) ⬆️
cms/factories.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4936cef...57a70fe. Read the comment docs.

@pdpinch
Copy link
Member

pdpinch commented May 8, 2019

sorry for the delay @asadiqbal08. Here's the design: https://invis.io/EFRWZWL4TZ5#/362236630_MITxPro_ReadMore_Modal

@@ -267,6 +294,7 @@ class Meta:
"ForTeamsPage",
"WhoShouldEnrollPage",
"CoursesInProgramPage",
"UserTestimonialsPage",
Copy link
Contributor

Choose a reason for hiding this comment

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

may be in next upcoming PR , re order the list according to .psd sections.

@@ -27,7 +27,7 @@
{% if course.current_price %}
<li>
<span class="title">PRICE</span>
<span class="text">${{ course.current_price }}</span>
<span class="text">${{ course.current_price|floatformat:"0" }}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks

</div>
</div>
{% endfor %}
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: here.

@@ -13,4 +13,5 @@
<glyph unicode="&#xe903;" glyph-name="arrow-circle-left" horiz-adv-x="878" d="M731.429 402.286v73.143c0 20-16.571 36.571-36.571 36.571h-286.857l108 108c6.857 6.857 10.857 16 10.857 25.714s-4 18.857-10.857 25.714l-52 52c-6.857 6.857-16 10.286-25.714 10.286s-18.857-3.429-25.714-10.286l-258.857-258.857c-6.857-6.857-10.286-16-10.286-25.714s3.429-18.857 10.286-25.714l258.857-258.857c6.857-6.857 16-10.286 25.714-10.286s18.857 3.429 25.714 10.286l52 52c6.857 6.857 10.286 16 10.286 25.714s-3.429 18.857-10.286 25.714l-108 108h286.857c20 0 36.571 16.571 36.571 36.571zM877.714 438.857c0-242.286-196.571-438.857-438.857-438.857s-438.857 196.571-438.857 438.857 196.571 438.857 438.857 438.857 438.857-196.571 438.857-438.857z" />
<glyph unicode="&#xe904;" glyph-name="arrow-circle-down" horiz-adv-x="878" d="M733.714 438.286c0 9.714-3.429 18.857-10.286 25.714l-52 52c-6.857 6.857-16 10.286-25.714 10.286s-18.857-3.429-25.714-10.286l-108-108v286.857c0 20-16.571 36.571-36.571 36.571h-73.143c-20 0-36.571-16.571-36.571-36.571v-286.857l-108 108c-6.857 6.857-16 10.857-25.714 10.857s-18.857-4-25.714-10.857l-52-52c-6.857-6.857-10.286-16-10.286-25.714s3.429-18.857 10.286-25.714l258.857-258.857c6.857-6.857 16-10.286 25.714-10.286s18.857 3.429 25.714 10.286l258.857 258.857c6.857 6.857 10.286 16 10.286 25.714zM877.714 438.857c0-242.286-196.571-438.857-438.857-438.857s-438.857 196.571-438.857 438.857 196.571 438.857 438.857 438.857 438.857-196.571 438.857-438.857z" />
<glyph unicode="&#xe905;" glyph-name="arrow-circle-up" horiz-adv-x="878" d="M733.714 439.428c0 9.714-3.429 18.857-10.286 25.714l-258.857 258.857c-6.857 6.857-16 10.286-25.714 10.286s-18.857-3.429-25.714-10.286l-258.857-258.857c-6.857-6.857-10.286-16-10.286-25.714s3.429-18.857 10.286-25.714l52-52c6.857-6.857 16-10.286 25.714-10.286s18.857 3.429 25.714 10.286l108 108v-286.857c0-20 16.571-36.571 36.571-36.571h73.143c20 0 36.571 16.571 36.571 36.571v286.857l108-108c6.857-6.857 16-10.857 25.714-10.857s18.857 4 25.714 10.857l52 52c6.857 6.857 10.286 16 10.286 25.714zM877.714 438.857c0-242.286-196.571-438.857-438.857-438.857s-438.857 196.571-438.857 438.857 196.571 438.857 438.857 438.857 438.857-196.571 438.857-438.857z" />
</font></defs></svg>
<glyph unicode="&#xe906;" glyph-name="close-outline" d="M150.016 98.816c-96.436 93.233-156.308 223.762-156.308 368.276 0 282.77 229.23 512 512 512 144.514 0 275.043-59.872 368.137-156.164l0.138-0.144c88.925-91.979 143.724-217.436 143.724-355.692 0-282.77-229.23-512-512-512-138.256 0-263.713 54.799-355.836 143.863l0.144-0.138zM222.208 171.008c74.164-74.164 176.621-120.036 289.792-120.036 226.342 0 409.828 183.486 409.828 409.828 0 113.171-45.872 215.628-120.036 289.792v0c-74.164 74.164-176.621 120.036-289.792 120.036-226.342 0-409.828-183.486-409.828-409.828 0-113.171 45.872-215.628 120.036-289.792v0zM729.088 605.696l-144.896-144.896 144.896-144.896-72.192-72.192-144.896 144.896-144.896-144.896-72.192 72.192 144.896 144.896-144.896 144.896 72.192 72.192 144.896-144.896 144.896 144.896 72.192-72.192z" />
</font></defs></svg>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: newline

.quote-container {
max-height: 50vh;
overflow: auto;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Newline


.headshot-image {
box-shadow: 0em 0.05em 0.5em rgba(0, 0, 0, 0.35);
border-width: 3px !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

why !important here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bootstrap border class used in conjunction with this already defines the border-width with !important so I need this to override it.

@@ -215,4 +215,4 @@
}
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

@asadiqbal08
Copy link
Contributor

@ahmed-arbisoft overall it LGTM 👍 few nits only.

@ahmed-arbisoft ahmed-arbisoft requested a review from pdpinch May 9, 2019 13:14
@ahmed-arbisoft ahmed-arbisoft assigned pdpinch and unassigned asadiqbal08 May 9, 2019
@ahmed-arbisoft ahmed-arbisoft force-pushed the ahmedbelal/145-product-page-learners-carousel branch from 8a5792d to b3e8870 Compare May 10, 2019 08:49
@ahmed-arbisoft ahmed-arbisoft force-pushed the ahmedbelal/145-product-page-learners-carousel branch from b3e8870 to e7981aa Compare May 11, 2019 16:16
@pdpinch pdpinch removed their assignment May 13, 2019
@pdpinch
Copy link
Member

pdpinch commented May 13, 2019

Hopefully we will get this Travis issue fixed by the time you get in tomorrow

@ahmed-arbisoft ahmed-arbisoft force-pushed the ahmedbelal/145-product-page-learners-carousel branch from e7981aa to 57a70fe Compare May 14, 2019 07:36
@ahmed-arbisoft ahmed-arbisoft merged commit ff614d9 into master May 14, 2019
@ahmed-arbisoft ahmed-arbisoft deleted the ahmedbelal/145-product-page-learners-carousel branch July 10, 2019 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Product Page: Learners carousel
5 participants