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

Inline Media Indicator #3029

Merged
merged 26 commits into from
Feb 4, 2020
Merged

Inline Media Indicator #3029

merged 26 commits into from
Feb 4, 2020

Conversation

j-pendlebury
Copy link
Contributor

@j-pendlebury j-pendlebury commented Jan 29, 2020

Resolves #3019

Overall change:
Add prop to MediaIndicator to be able to use this component inline with text.

Code changes:

  • Add isInline prop to MediaIndicator
  • Update stories for each type - video, audio, photogallery
  • Add knob to stories for inline MediaIndicator to toggle inline state, and to add extra text if required
  • Add tests for inline media indicator for LTR and RTL service (only for video since the behaviour is the same for other types)

Index Also with isInline=true on News

image

Index Also with isInline=true on Persian

image

IE11

image
image

(with Persian, the behaviour is different on IE11 than Chrome, but I checked on latest and it's the exact same behaviour)


  • 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

@j-pendlebury j-pendlebury self-assigned this Jan 29, 2020
@j-pendlebury j-pendlebury added the ws-home Tasks for the WS Home Team label Jan 29, 2020
@sareh sareh mentioned this pull request Jan 29, 2020
6 tasks
@j-pendlebury j-pendlebury force-pushed the inline-media-indicator branch from 3d28f65 to 6e91f86 Compare January 30, 2020 14:08
@sareh
Copy link
Contributor

sareh commented Jan 30, 2020

@jblaut That IE11 issue is to do with the text itself not being in a RTL script. When I replaced it with arabic text, it displayed correctly:
Screenshot 2020-01-30 at 15 25 04

I will open an issue to fix the Storybook test for StoryPromo with index alsos, using the Storybook helper to use the correct localised text for the index alsos. It'd make testing index alsos reflective of reality.

@@ -42,54 +49,44 @@ const TimeDuration = styled.time`
margin: 0 ${GEL_SPACING_HLF};
`;

const IndexAlsosMediaIndicator = styled.span`
Copy link
Contributor

@DenisHdz DenisHdz Jan 30, 2020

Choose a reason for hiding this comment

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

We will have to also update the IndexAlsos straight after to use the MediaIndicatorWrapper with the isInline prop instead since this IndexAlsosMediaIndicator won't exist anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created Psammead and Simorgh issues 👍


MediaIndicator.propTypes = {
datetime: string,
duration: string,
type: oneOf(['video', 'audio', 'photogallery']),
topStory: bool,
service: string.isRequired,
indexAlsos: bool,
isInline: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should include a small update to the README if we're adding or removing props 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All done 👍

Copy link
Contributor

@OlgaLyubin OlgaLyubin left a comment

Choose a reason for hiding this comment

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

Older stories ("audio without duration", "video without duration" and "photogallery") have indexAlsos prop. Other than that everything looks great

@j-pendlebury
Copy link
Contributor Author

Older stories ("audio without duration", "video without duration" and "photogallery") have indexAlsos prop. Other than that everything looks great

all fixed now 👍

Copy link
Contributor

@OlgaLyubin OlgaLyubin left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@PriyaKR
Copy link
Contributor

PriyaKR commented Feb 4, 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 component
9 participants