Conversation
src/app/components/Byline/index.tsx
Outdated
| case ARTICLE_PAGE: | ||
| return ( | ||
| <ArticleContributors | ||
| contributorValues={contributorValues} | ||
| isSingleContributor={isSingleContributor} | ||
| /> | ||
| ); | ||
| case MEDIA_ARTICLE_PAGE: | ||
| return ( | ||
| <ArticleContributors |
There was a problem hiding this comment.
As they are returning the same thing, should we combine the Article and Media Article cases here? Or have the response as the default?
| imageLtr: () => | ||
| css([ | ||
| { | ||
| float: 'left', | ||
| marginRight: `${pixelsToRem(8)}rem`, | ||
| }, | ||
| ]), | ||
|
|
||
| imageRtl: () => | ||
| css([ | ||
| { | ||
| float: 'right', | ||
| marginLeft: `${pixelsToRem(8)}rem`, | ||
| }, | ||
| ]), |
There was a problem hiding this comment.
Instead of using direction to apply these styles, we should use inline values https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/float#values
https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/margin-inline
| displayBlock: () => | ||
| css({ | ||
| display: `block`, | ||
| }), | ||
|
|
||
| displayFlex: () => | ||
| css({ | ||
| display: `flex`, | ||
| alignItems: 'center', | ||
| }), |
There was a problem hiding this comment.
Interesting pattern, we should probably stick to what we normally do, and name these according to the elements they're going to be used on, like unorderedList and listSpan
| jobRole: ({ palette, isDarkUi }: Theme) => | ||
| css({ color: isDarkUi ? palette.GREY_2 : palette.GREY_6 }), | ||
|
|
||
| ImageWrapper: ({ palette }: Theme) => |
There was a problem hiding this comment.
| ImageWrapper: ({ palette }: Theme) => | |
| imageWrapper: ({ palette }: Theme) => |
| </li> | ||
| )} | ||
| <li css={hasMultipleContributors && BylineCss.displayInline}> | ||
| {authorTopicUrl ? ( |
There was a problem hiding this comment.
This whole component is getting quite hard to read at first glance.
Maybe we extract ternary parts of the components into subcomponents outside of the return function, and in the main body of the component, so for example we move this to something like AuthorTopicLink, and use the same pattern for other ternary parts of the component. What do you think?
3 participants
Resolves JIRA: WS-1985
Summary
Add a byline to Live Page posts
Code changes
NOTES
yarn test:dependenciesfails when the import format below is used hence the need for the// eslint-disable-next-line import/no-relative-packagesexclusions.Testing
Useful Links