Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

Use updated media indicator in Story Promo and Index Alsos #3073

Merged
merged 47 commits into from
Feb 10, 2020

Conversation

sareh
Copy link
Contributor

@sareh sareh commented Feb 6, 2020

Resolves #3039

Overall change: Use updated media indicator in Story Promo Index Alsos
Update stories in Psammead Grid, Psammead Story Promo to use updated major version of Media Indicator @bbc/psammead-media-indicator@4

Storybook Story Promo with Audio story
Screen Shot 2020-02-07 at 08 20 38

Storybook Regular Promo No image with Video
Screen Shot 2020-02-07 at 08 50 57

Grid example with Story Promos with Media indicators

Note: the lack of padding here will only be fixed once the psammead-story-promo package update here is pulled into top-level psammead as a dependency.
Screen Shot 2020-02-07 at 08 49 52

Code changes:

  • Update to use @bbc/[email protected]
  • Using the isInline prop to switch between block and inline versions
  • Ensuring headlines etc that are inline with the media indicator have the display: inline property so they wrap around the media indicator and below the icon, as per designs.

Testing notes


  • I have assigned myself to this PR and the corresponding issues
  • Automated jest tests added (for new features) or updated (for existing features)
  • This PR requires manual testing

@sareh sareh added ws-front-page-layout ws-home Tasks for the WS Home Team labels Feb 6, 2020
@sareh sareh self-assigned this Feb 6, 2020
CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: Einstein Njoroge  <[email protected]>
@@ -3,6 +3,7 @@
<!-- prettier-ignore -->
| Version | Description |
|---------|-------------|
| 1.1.7 | [PR#3073](https://github.com/bbc/psammead/pull/3073) Update Grid Stories to include new major version of MediaIndicator |
Copy link
Contributor

@DenisHdz DenisHdz Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this PR gets merged, we have to bump the psammead-story-promo in the root package.json, to make sure these changes are applied correctly.

@@ -75,9 +77,14 @@ Info.defaultProps = {
dir: 'ltr',
promoHasImage: true,
};

const TimeDuration = styled.time`
Copy link
Contributor

@DenisHdz DenisHdz Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@DenisHdz DenisHdz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

…s will be added when the updated psammead-story-promo is pulled in at the top-level psammead directory to be used in stories
Copy link
Contributor Author

@sareh sareh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just adding notes to each change, for reference when Code Reviewing.

const mediaInfo = (
<MediaIndicator duration="2:15" datetime="PT2M15S" service="news" />
<MediaIndicator script={latin} service="news">
<StyledTime datetime="PT2M15S">2:15</StyledTime>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating to pass in a Time component into Media Indicator in our test.

isInline={mediaIndicatorIsInline}
>
{!mediaIndicatorIsInline && (
<StyledTime dateTime="PT2M15S">2:15</StyledTime>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating to pass in a Time component to our Media Indicator for these stories.

@@ -110,6 +134,9 @@ export const Headline = styled.h3`
padding-bottom: ${GEL_SPACING};
${({ service }) => getSerifMedium(service)}
${({ script, promoType }) => script && headlineTypography(script)[promoType]}
${({ promoHasImage }) =>
!promoHasImage &&
`display: inline;`} /* Needed for aligning Media Indicator with Headline */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If promo does not have an image, then the headline needs to be displayed inline with the media indicator.

${({ displayImage }) =>
!displayImage &&
`>div{ display:inline-block; padding: 0; vertical-align:initial; } `}
`>div{ display:inline-block; vertical-align:initial; }
& svg{ margin: 0; }`}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resetting margin on the Media Icon svg to 0 when there is no image (e.g. the media indicator is inline)

@@ -54,24 +48,23 @@ const StyledIndexAlsosLink = styled.a`
&:visited {
color: ${C_METAL};
}

& svg { margin: 0; } /* Reset Media Indicator SVG margin */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting margin on the svg within an index also to 0.

@PriyaKR PriyaKR self-assigned this Feb 10, 2020
@PriyaKR
Copy link
Contributor

PriyaKR commented Feb 10, 2020

Looks good to me.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Media Indicator in Index Alsos
5 participants