-
Notifications
You must be signed in to change notification settings - Fork 0
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
WEB-6415: Add module outcomes details #217
Conversation
KapilSachdev
commented
Oct 2, 2023
- And pass on to alexandria
- Add text content ref details
app/models/text.rb
Outdated
@@ -27,7 +27,7 @@ def slug | |||
|
|||
# Used for serialisation | |||
def attributes | |||
{ title: nil, ordinal: nil, description: nil, body: nil, authors: [], free: false, episode_type: nil }.stringify_keys | |||
{ title: nil, ordinal: nil, description: nil, body: nil, authors: [], free: false, episode_type: nil, ref: nil }.stringify_keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this here be something else than episode_type
?
You've changed episode
to segment
above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can. I retained the existing naming of episode here for now rather than creating a new segment_type as it uses existing parser of video, chapter and assessment.
I think i should include it, just to be safe.
app/models/content_module.rb
Outdated
:covered_concepts_md, :authors, :lessons, :git_commit_hash, :card_artwork_image, | ||
:featured_banner_image, :twitter_card_image, :root_path, :access_personal, | ||
:access_team | ||
|
||
attr_markdown :who_is_this_for, source: :who_is_this_for_md, file: false | ||
attr_markdown :covered_concepts, source: :covered_concepts_md, file: false | ||
attr_markdown :outcomes, source: :module_outcomes_md, file: false | ||
attr_markdown :description, source: :description_md, file: false | ||
attr_image :card_artwork_image_url, source: :card_artwork_image, variants: %i[original w560 w240] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing to do with the actual review, but I think we talked about not requiring card_artwork_image
anymore, as it is identical to feature_banner
after rebranding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jellodiil Can you please also suggest what artwork would be there for content modules? or lesson or segment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think it is just feature_banner
and twitter_card
.
Feature_banner will be used everywhere where card_artwork
used to be (they used to be two slightly different images, but now its the same image for both, but I think we exported those two at different sizes).
- Identical with feature_banner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 looks ok to me