-
Notifications
You must be signed in to change notification settings - Fork 153
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
Update Spotlight
homepage posts to Ideas
, including cta link text
#12663
Update Spotlight
homepage posts to Ideas
, including cta link text
#12663
Conversation
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.
Great work on this @ramram-mf! Code looks good, and solves the issue. But I had a couple of thoughts regarding the long-term of this element.
First, I noticed we still have mixed use of Spotlight image
and Ideas image
, etc. (see screenshot below). If we are moving to Ideas
I wonder if we should rename it in full (templates & models too) to avoid confusion with this element in the future.
The second thought I had was if we're adding a cta
link text, maybe we just should migrate the blog
post and cta
fields to just use to the new LinkBlock
field (the one @danielfmiranda has been migrating a bunch the past couple sprints). For example, we had a request come in last week to link these sections to a PNI article page, but since these fields are strictly limited to a Blog Page
, we weren't able to accommodate the request. If it were a LinkBlock
, we'd have a lot more flexibility to do that and also include the "Read More" cta
text.
Curious to hear your thoughts!
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.
@robdivincenzo So, I explored changing the |
…b-mf:MozillaFoundation/foundation.mozilla.org into TP1-981-add-text-links-to-ideas-section-cards
@ramram-mf The text links seem to be missing on mobile, is that intentional? |
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.
Ty for cleaning up the naming and looking into the LinkBlock
idea, code LGTM! 🚀
@nancyt1 Updated the mobile layout so it now looks like this: |
Description
This PR updates the homepage
Spotlight
posts, renaming the block toIdeas
and adding aCTA Link Text
field, which is displayed on the lower part of the post component.Current state:
Updated state:
# To test
Ideas
card. Publish.Ideas
header and CTA latest values.Link to sample test page: https://foundation-s-tp1-981-ad-tfr9ki.herokuapp.com/
Related PRs/issues: #12601
┆Issue is synchronized with this Jira Story