Byline improvements #193
Byline improvements #193
Conversation
philmcmahon
requested review from
AWare,
GHaberis,
nicl,
SiAdcock and
sndrs
as code owners
frontend/components/ArticleBody.tsx
Outdated
| @@ -259,14 +276,39 @@ const metaExtras = css` | |||
|
|
|||
| const pillarColour = palette.lifestyle.main; // TODO make dynamic | |||
|
|
|||
| const dtFormat = (date: Date) => dateformat(date, 'ddd d mmm yyyy HH:MM "GMT"'); | |||
| const dtFormat = (date: Date) => dateformat(date, 'ddd d mmm yyyy HH:MM Z'); | |||
There was a problem hiding this comment.
TypeScript question, do we need to define the return type of dtFormat or can TypeScript implicitly deduce it's type? I'm guessing in this case it's a string but I can't see any type definitions for dateformat.
There was a problem hiding this comment.
it compiled so...I guess it deduced the type? I've added the return type in, I find in scala it's generally better to define a return type
| paidContentType?: string | ||
| } | ||
|
|
||
| interface TagType { |
There was a problem hiding this comment.
Looks like we're going to have a conflict of sorts here as I've also defined a new TagType in my branch https://github.com/guardian/dotcom-rendering/pull/192/files#diff-978893e81256a8dbee76e6fe1045f2d7R28. I'm happy to hang on merging mine until this is in and I'll use this new type, as my TagType is pretty basic.
There was a problem hiding this comment.
if that's ok then great! happy to wait for you as well though
frontend/components/ArticleBody.tsx
Outdated
| <div className={profile(pillarColour)}> | ||
| <span | ||
| className={byline} | ||
| dangerouslySetInnerHTML={{ |
There was a problem hiding this comment.
I wouldn't use dangerouslySetInnerHTML with renderByline returning a string here, as this mixing markup in a string template with JSX markup. You could replace renderByline with a function that returns a stateless React component. If you call this function Byline for example you can then put <Byline byline={byline} tags={CAPI.tags} /> as a child of this span (not you can pass byline and tags as props.
There was a problem hiding this comment.
👆
@philmcmahon I'd be happy to pair with you on this today if you like
There was a problem hiding this comment.
Gosh, this is more complicated than it looks. I'd say don't worry about this for now, it can be an interesting coding challenge for us one day!
There was a problem hiding this comment.
I think there's a good case for turning this into an array of structured data and then mapping that.
[ 'Firstname Lastname', 'and', {?:'Other Name',url:'https://'} ]
frontend/components/ArticleBody.tsx
Outdated
| </div> | ||
| {CAPI.author.twitterHandle && ( | ||
| <div className={twitterHandle}> | ||
| <TwitterIcon />{' '} |
There was a problem hiding this comment.
Is this {' '} required? If we're using it to put space between <TwitterIcon /> and the link then we should use css to achieve this affect.
frontend/lib/parse-capi/index.ts
Outdated
| author: getString(data, 'config.page.author'), | ||
| author: { | ||
| byline: getString(data, 'config.page.byline', ''), | ||
| twitterHandle: leadContributor.properties.twitterHandle, |
There was a problem hiding this comment.
If there are no tags where tagType === 'Contributor' then leadContributor would be undefined, if it is looking up properties on it'll cause an error. You could add a ternary to check if it's defined and if not just assign undefined to twitterHandle. eg.
twitterHandle: leadContributor ? leadContributor.properties.twitterHandle : undefined,.
Same goes for email below...
philmcmahon
force-pushed
the
pm-more-forgiving-validation
branch
from
b8ee163 to
0814f68
Compare
|
sorry I've been so slow finishing this off! I'm going to implement @AWare 's suggested change and then get this merged |
|
Thanks @philmcmahon. Gizza wave if you want to pair 👋 |
philmcmahon
force-pushed
the
pm-more-forgiving-validation
branch
from
3e1f3c6 to
5fa4214
Compare
frontend/lib/parse-capi/index.ts
Outdated
| @@ -278,17 +278,22 @@ export const extractArticleMeta = (data: {}): CAPIType => { | |||
| const webPublicationDate = new Date( | |||
| getNumber(data, 'config.page.webPublicationDate'), | |||
| ); | |||
| const tags = getTags(data); | |||
| const tags = getArray<TagType>(data, 'tags.tags'); | |||
There was a problem hiding this comment.
If an article doesn't have tags.tags then we'll not get a 500, I don't think we should be so strict, previously getTags would've returned an empty array if no tags were available.
What does this change?
Why?
Cos it looks better this way - before:
after:
#What's with that dangerouslysethtml?
I read all of this thread on replacing bits of string with react templates, and decided that this was the only way to make it work