-
Notifications
You must be signed in to change notification settings - Fork 17
CULT-747 #2356
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
base: main
Are you sure you want to change the base?
Conversation
d7cdec1 to
39f7fbb
Compare
| @mixin _oTeaserByline { | ||
| @include _oTeaserLink(); | ||
| font-family: oPrivateFoundationGet('o3-font-family-metric'); | ||
| font-size: oPrivateFoundationGet('o3-font-size-metric2-0'); |
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.
TODO: check with Jazmin about font size depends on the size of teasers.
95bd28d to
f2ec30f
Compare
9fd4891 to
2f35866
Compare
| The Byline component already includes a headshot. | ||
| Only use the headshot when the media rule `headshot` is true | ||
| and `showByline` is falsy. */} | ||
| {media(props) === 'headshot' && !props.showByline ? <Headshot {...props} /> : null} |
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.
I think we shouldn't attempt render the old headshot component if there is new byline data:
{media(props) === 'headshot' && !props.byline?.length ? <Headshot {...props} /> : null}
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.
Don't we need to give the choice until we remove the headshot from x-teaser & o-teaser?
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.
I didn’t include !props.byline?.length in the condition because we don’t know how this component is used. It could be used with our Elasticsearch teaser data, and consumers may pass either old or new data.
Instead I’ve set the default in the preset to showByline: true. If a consumer wants to keep the old headshot behaviour, they’ll need to explicitly set showByline: false. This is to avoid displaying mixed teasers with the old headshot and the new byline.
| /> | ||
| ); | ||
|
|
||
| export default ({showByline, byline, showBylineHeadshot, headshot}: BylineProps) => { |
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.
Shouldn't we be ignoring headshot here? Presumably if this data is set it's old data. We only want to render headshots contained in the first element of the structured byline.
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.
showBylineHeadshot - can't we use the existing showHeadshot here? I think we should just respect that because it probably means the teaser shouldn't display this data because it doesn't fit.
Likewise, potentially we could be saying that if showHeadshot === false and showByline is undefined (or null) then set showByline = showHeadshot... because they probably mean the same thing. If that turns out to be wrong a for specific teaser slot then the application code can be changed. But if we don't do this then we would start seeing bylines where they weren't intended.
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.
showBylineHeadshot- can't we use the existingshowHeadshothere? I think we should just respect that because it probably means the teaser shouldn't display this data because it doesn't fit.
That is a interesting point. That makes sense. I will change that.
Likewise, potentially we could be saying that if showHeadshot === false and showByline is undefined (or null) then set showByline = showHeadshot... because they probably mean the same thing. If that turns out to be wrong a for specific teaser slot then the application code can be changed. But if we don't do this then we would start seeing bylines where they weren't intended.
I'm not sure this point. They are completely different slot and the tag won't display author name anymore. I would like to talk about this more.
| const Headshot = ({headshot}: {headshot: string}) => ( | ||
| <img | ||
| className="o-teaser__byline__headshot" | ||
| width={ImageSizes.BylineHeadshot} |
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.
Are width and height attributes essential here? Shouldn't these be set in CSS?
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.
I think we need to pass them to create the origami image service url.
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.
sorry, I misunderstood the question. I just copied the code from Headshot component. What will be the difference set here or css?
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.
I searched and it seems even if the size is set in CSS, setting width and height on <img> helps the browser calculate the aspect ratio earlier and prevents layout shift. For a fixed-size, non-responsive image, it’s recommended to keep them.
3b4aea5 to
04bceaf
Compare
The author headshot has been moved into a new Byline component. The latest design no longer displays the headshot on the right side. The legacy headshot component is still kept for consumers that need it. By default, showByline is set to true in presets, so the new component is applied automatically. Consumers can set showByline to false to continue using the legacy headshot behaviour. To migrate to the new Byline component, teaser data must include a byline property in the following format: `byline: [[ AuthorNameString, LinkUrlString?, HeadshotString?]]`
Customer Product Curation & Loyalty team is working on Opinion teaser improvements. The PRD
To achieve the improvement, x-teaser & o-teaser need to handle new teaser properties.
bylinestreamLinkstitlePrefix.*conceptLink in the image was an early name for streamLink. The link is not limited to concepts and may also point to an article package landing page, which is why streamLink was chosen instead.
StreamLink
showStreamLinktruestreamLinks[streamLinklabel, streamLinkUrl]streamLinksproperty whenshowStreamLinkistrueandstreamLinksis set.showStreamLinkisfalse, or thestreamLinksproperty isundefined, it falls back tometaLinkandmetaPrefix.TitlePrefix
showTitlePrefixtruetitlePrefixshowTitleandshowTitlePrefixaretrue, and thetitlePrefixproperty is set.TitleIndicator
showTitleIndicatortrueshowTitleandshowTitleIndicatoraretrue, and the content is targeted.Currently, only Opinion teasers with
indicators.isOpinion: truedisplay the speech icon.Byline
Caution
Breaking change!!
The Byline component is enabled by default, as
showBylineis set totrue.This means the Byline will be rendered instead of the legacy Headshot component when byline data is available.
If your application is not ready to adopt the Byline component yet, it can opt out by explicitly setting
showBylinetofalse.In that case, the existing Headshot behaviour will remain unchanged.
showBylinetruebyline[authorName, linkUrl, headshot]showBylineistrueand thebylineproperty is set.showBylineandshowHeadshotaretrue, and thebylineentry includes a headshot value (index 2).showBylinetakes precedence overshowHeadshot, so it will NOT fall back to a headshot even if old (withoutbyline) and new content (withbyline) models exist on the same page.