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

Add news and events carousel #2111

Merged
merged 2 commits into from
Feb 11, 2021

Conversation

HamzaIbnFarooq
Copy link
Contributor

@HamzaIbnFarooq HamzaIbnFarooq commented Feb 4, 2021

Pre-Flight checklist

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

What are the relevant tickets?

#2108

What's this PR do?

Adds a NEWS/EVENTS section to HomePage.

How should this be manually tested?

  • Go to cms
  • Under HomePage create a News and Events Page
  • Add the required fields and save
  • A new section should appear below testimonial on your HomePage

Screenshots (if appropriate)

Desktop:
image

Mobile:
image

@odlbot odlbot temporarily deployed to xpro-ci-pr-2111 February 4, 2021 12:46 Inactive
@codecov-io
Copy link

codecov-io commented Feb 4, 2021

Codecov Report

Merging #2111 (c87bd72) into master (3b19dc7) will increase coverage by 0.01%.
The diff coverage is 90.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2111      +/-   ##
==========================================
+ Coverage   87.96%   87.97%   +0.01%     
==========================================
  Files         328      328              
  Lines       14778    14819      +41     
  Branches     1032     1033       +1     
==========================================
+ Hits        12999    13037      +38     
- Misses       1540     1542       +2     
- Partials      239      240       +1     
Impacted Files Coverage Δ
cms/models.py 90.78% <80.00%> (-0.44%) ⬇️
cms/blocks.py 83.67% <100.00%> (+2.72%) ⬆️
cms/factories.py 94.47% <100.00%> (+0.38%) ⬆️
...tic/js/components/forms/BulkEnrollmentForm_test.js 100.00% <0.00%> (+1.26%) ⬆️

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 3b19dc7...7a233ef. Read the comment docs.

@odlbot odlbot temporarily deployed to xpro-ci-pr-2111 February 4, 2021 13:50 Inactive
@HamzaIbnFarooq HamzaIbnFarooq force-pushed the hamza/2108-news_and_events_carousel branch from d2a36d4 to 8c48bc9 Compare February 4, 2021 13:52
@odlbot odlbot temporarily deployed to xpro-ci-pr-2111 February 4, 2021 13:53 Inactive
@HamzaIbnFarooq HamzaIbnFarooq changed the title Hamza/2108 news and events carousel Add news and events carousel Feb 4, 2021
@HamzaIbnFarooq HamzaIbnFarooq force-pushed the hamza/2108-news_and_events_carousel branch from 8c48bc9 to a27a056 Compare February 8, 2021 06:17
@odlbot odlbot temporarily deployed to xpro-ci-pr-2111 February 8, 2021 06:18 Inactive
@briangrossman
Copy link
Contributor

@HamzaIbnFarooq
Screenshots look good to me (and Abdul).
Nice work!

@odlbot odlbot temporarily deployed to xpro-ci-pr-2111 February 9, 2021 10:11 Inactive
@asadiqbal08 asadiqbal08 self-assigned this Feb 9, 2021
Copy link
Contributor

@asadiqbal08 asadiqbal08 left a comment

Choose a reason for hiding this comment

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

Over all it seems me great beside few suggestions.

cms/blocks.py Outdated
Custom block to represent a news or event
"""

content_type = blocks.CharBlock(max_length=100, help_text="The type of update.")
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: I would prefer Specify the news/events type 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.

updated.

cms/blocks.py Outdated
"""

content_type = blocks.CharBlock(max_length=100, help_text="The type of update.")
title = blocks.CharBlock(max_length=255, help_text="The title of update.")
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: I would prefer Specify the news/events title 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.

updated.

cms/blocks.py Outdated
content_type = blocks.CharBlock(max_length=100, help_text="The type of update.")
title = blocks.CharBlock(max_length=255, help_text="The title of update.")
image = ImageChooserBlock(
blank=True, null=True, help_text="The image to display on the update."
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: I would prefer Specify the image for news/events section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

cms/blocks.py Outdated
call_to_action = blocks.CharBlock(
max_length=100, help_text="The call to action of update."
)
action_url = blocks.URLBlock(help_text="The call to action url of update.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add an example here for the action_url , I mean a bit more specific to new ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

blank=True, null=True, help_text="The image to display on the update."
)
content = blocks.TextBlock(help_text="The content that appears on the update card.")
call_to_action = blocks.CharBlock(
Copy link
Contributor

Choose a reason for hiding this comment

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

a bit more explanatory with any possible e.g. value ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

<img src="{% image_version_url card.value.image "fill-330x200" %}" alt="{{ card.value.name }}" width="330" height="200" loading="lazy"/>
{% endif %}
<div class="card-type">
{{ card.value.content_type }}
Copy link
Contributor

Choose a reason for hiding this comment

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

are we considering to keep these values required in CMS ? I just mean, if no any data then we may have empty HTML tags 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.

All fields are mandatory right now, according to the requirements.

{{ card.value.content_type }}
</div>
<p class="title">{{ card.value.title }}</p>
<p class="content">{{ card.value.content|truncatechars:150 }}</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we considering to keep these values required in CMS ? I just mean, if no any data then we may have empty HTML tags 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.

All fields are mandatory right now, according to the requirements.

@odlbot odlbot temporarily deployed to xpro-ci-pr-2111 February 9, 2021 12:45 Inactive
@HamzaIbnFarooq HamzaIbnFarooq force-pushed the hamza/2108-news_and_events_carousel branch from 8b6795d to cc38d4c Compare February 9, 2021 12:50
@odlbot odlbot temporarily deployed to xpro-ci-pr-2111 February 9, 2021 12:50 Inactive
@HamzaIbnFarooq HamzaIbnFarooq force-pushed the hamza/2108-news_and_events_carousel branch from cc38d4c to c87bd72 Compare February 9, 2021 12:52
@odlbot odlbot temporarily deployed to xpro-ci-pr-2111 February 9, 2021 12:52 Inactive
@HamzaIbnFarooq HamzaIbnFarooq force-pushed the hamza/2108-news_and_events_carousel branch from c87bd72 to 7a233ef Compare February 9, 2021 13:56
@odlbot odlbot temporarily deployed to xpro-ci-pr-2111 February 9, 2021 13:56 Inactive
@HamzaIbnFarooq
Copy link
Contributor Author

@asadiqbal08 I have done a small design change of removing icomoon usage in this PR, kindly have a look again to test the changes.

@HamzaIbnFarooq
Copy link
Contributor Author

HamzaIbnFarooq commented Feb 10, 2021

@briangrossman can you please re-verify the attached design as I have replaced icomoon icons with material design ones, according to this PR #2093

@briangrossman
Copy link
Contributor

@HamzaIbnFarooq Thanks for pointing this out. Those icons look totally fine.

@HamzaIbnFarooq HamzaIbnFarooq merged commit 8ae2481 into master Feb 11, 2021
@HamzaIbnFarooq HamzaIbnFarooq deleted the hamza/2108-news_and_events_carousel branch February 11, 2021 03:57
@odlbot odlbot mentioned this pull request Feb 11, 2021
7 tasks
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