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

Media Indicator - remove spacing on the low-level component #3062

Merged
merged 15 commits into from
Feb 6, 2020
+88 −338
Merged

Media Indicator - remove spacing on the low-level component #3062

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
15981da
Remove spacing on the low-level component
DenisHdz Feb 5, 2020
302a585
Update PR number
DenisHdz Feb 5, 2020
18026c0
Remove topStory prop
DenisHdz Feb 5, 2020
203483f
Update README
DenisHdz Feb 5, 2020
179d1d1
Update snapshots
DenisHdz Feb 5, 2020
6a6ddc9
Remove the Time element from the Media Indicator
DenisHdz Feb 5, 2020
bf5082d
Update stories
DenisHdz Feb 5, 2020
da3b757
Merge branch 'latest' into media-indicator-remove-spacing
OlgaLyubin Feb 5, 2020
f57c6f5
Remove story and fix typo in CHANGELOG
DenisHdz Feb 5, 2020
df44fdc
Merge branch 'media-indicator-remove-spacing' of github.com:bbc/psamm…
DenisHdz Feb 5, 2020
b103f1a
Merge branch 'latest' of github.com:bbc/psammead into media-indicator…
DenisHdz Feb 5, 2020
2ddfd9d
Merge branch 'latest' into media-indicator-remove-spacing
sareh Feb 6, 2020
74a70ae
Apply CR feedback
DenisHdz Feb 6, 2020
7fd71fe
Merge branch 'media-indicator-remove-spacing' of github.com:bbc/psamm…
DenisHdz Feb 6, 2020
eacc57d
Update packages/components/psammead-media-indicator/src/index.stories…
DenisHdz Feb 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/psammead-media-indicator/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<!-- prettier-ignore -->
| Version | Description |
| ------- | ----------- |
| 4.0.0 | [PR#3062](https://github.com/bbc/psammead/pull/3062) Remove spacing on the low-level component |
DenisHdz marked this conversation as resolved.
Show resolved Hide resolved
| 3.0.0 | [PR#3029](https://github.com/bbc/psammead/pull/3029) Add prop `isInline` for displaying media indicator inline. Remove boolean prop `indexAlsos` since it should be replaced with `isInline` |
| 2.6.27 | [PR#3030](https://github.com/bbc/psammead/pull/3030) Talos - Bump Dependencies - @bbc/psammead-styles, @bbc/psammead-assets |
| 2.6.26 | [PR#2996](https://github.com/bbc/psammead/pull/2996) Talos - Bump Dependencies - @bbc/psammead-assets |
Expand Down
2 changes: 1 addition & 1 deletion packages/components/psammead-media-indicator/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/components/psammead-media-indicator/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@bbc/psammead-media-indicator",
"version": "3.0.0",
"version": "4.0.0",
"main": "dist/index.js",
"module": "esm/index.js",
"sideEffects": false,
Expand Down
110 changes: 9 additions & 101 deletions packages/components/psammead-media-indicator/src/__snapshots__/index.test.jsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,12 @@ exports[`MediaIndicator - Top Story should render a top story audio promo render
}

.c0 {
padding: 0.5rem 0.25rem;
color: #222222;
background-color: #FFFFFF;
display: block;
font-family: ReithSans,Helvetica,Arial,sans-serif;
font-weight: 400;
font-style: normal;
font-size: 0.75rem;
line-height: 1rem;
color: #222222;
height: 2rem;
}

.c1 {
Expand Down Expand Up @@ -83,16 +79,12 @@ exports[`MediaIndicator should render audio correctly without duration details 1
}

.c0 {
padding: 0.5rem 0.25rem;
color: #222222;
background-color: #FFFFFF;
display: block;
font-family: ReithSans,Helvetica,Arial,sans-serif;
font-weight: 400;
font-style: normal;
font-size: 0.75rem;
line-height: 1rem;
color: #222222;
height: 2rem;
}

.c1 {
Expand All @@ -107,13 +99,6 @@ exports[`MediaIndicator should render audio correctly without duration details 1
height: 100%;
}

@media (max-width:24.9375rem) {
.c0 {
height: 1.25rem;
padding: 0.25rem 0.25rem 0;
}
}

<div
aria-hidden="true"
class="c0"
Expand Down Expand Up @@ -151,16 +136,12 @@ exports[`MediaIndicator should render audio indicator correctly 1`] = `
}

.c0 {
padding: 0.5rem 0.25rem;
color: #222222;
background-color: #FFFFFF;
display: block;
font-family: ReithSans,Helvetica,Arial,sans-serif;
font-weight: 400;
font-style: normal;
font-size: 0.75rem;
line-height: 1rem;
color: #222222;
height: 2rem;
}

.c1 {
Expand All @@ -180,13 +161,6 @@ exports[`MediaIndicator should render audio indicator correctly 1`] = `
margin: 0 0.25rem;
}

@media (max-width:24.9375rem) {
.c0 {
height: 1.25rem;
padding: 0.25rem 0.25rem 0;
}
}

<div
aria-hidden="true"
class="c0"
Expand Down Expand Up @@ -230,16 +204,12 @@ exports[`MediaIndicator should render photogallery correctly 1`] = `
}

.c0 {
padding: 0.5rem 0.25rem;
color: #222222;
background-color: #FFFFFF;
display: block;
font-family: ReithSans,Helvetica,Arial,sans-serif;
font-weight: 400;
font-style: normal;
font-size: 0.75rem;
line-height: 1rem;
color: #222222;
height: 2rem;
}

.c1 {
Expand All @@ -254,13 +224,6 @@ exports[`MediaIndicator should render photogallery correctly 1`] = `
height: 100%;
}

@media (max-width:24.9375rem) {
.c0 {
height: 1.25rem;
padding: 0.25rem 0.25rem 0;
}
}

<div
aria-hidden="true"
class="c0"
Expand Down Expand Up @@ -300,16 +263,12 @@ exports[`MediaIndicator should render video by default 1`] = `
}

.c0 {
padding: 0.5rem 0.25rem;
color: #222222;
background-color: #FFFFFF;
display: block;
font-family: ReithSans,Helvetica,Arial,sans-serif;
font-weight: 400;
font-style: normal;
font-size: 0.75rem;
line-height: 1rem;
color: #222222;
height: 2rem;
}

.c1 {
Expand All @@ -324,13 +283,6 @@ exports[`MediaIndicator should render video by default 1`] = `
height: 100%;
}

@media (max-width:24.9375rem) {
.c0 {
height: 1.25rem;
padding: 0.25rem 0.25rem 0;
}
}

<div
aria-hidden="true"
class="c0"
Expand Down Expand Up @@ -365,16 +317,12 @@ exports[`MediaIndicator should render video correctly without duration details 1
}

.c0 {
padding: 0.5rem 0.25rem;
color: #222222;
background-color: #FFFFFF;
display: block;
font-family: ReithSans,Helvetica,Arial,sans-serif;
font-weight: 400;
font-style: normal;
font-size: 0.75rem;
line-height: 1rem;
color: #222222;
height: 2rem;
}

.c1 {
Expand All @@ -389,13 +337,6 @@ exports[`MediaIndicator should render video correctly without duration details 1
height: 100%;
}

@media (max-width:24.9375rem) {
.c0 {
height: 1.25rem;
padding: 0.25rem 0.25rem 0;
}
}

<div
aria-hidden="true"
class="c0"
Expand Down Expand Up @@ -430,16 +371,12 @@ exports[`MediaIndicator should render video indicator correctly 1`] = `
}

.c0 {
padding: 0.5rem 0.25rem;
color: #222222;
background-color: #FFFFFF;
display: block;
font-family: ReithSans,Helvetica,Arial,sans-serif;
font-weight: 400;
font-style: normal;
font-size: 0.75rem;
line-height: 1rem;
color: #222222;
height: 2rem;
}

.c1 {
Expand All @@ -459,13 +396,6 @@ exports[`MediaIndicator should render video indicator correctly 1`] = `
margin: 0 0.25rem;
}

@media (max-width:24.9375rem) {
.c0 {
height: 1.25rem;
padding: 0.25rem 0.25rem 0;
}
}

<div
aria-hidden="true"
class="c0"
Expand Down Expand Up @@ -506,16 +436,12 @@ exports[`MediaIndicator should render video indicator correctly when inline 1`]
}

.c0 {
padding: 0.5rem 0.25rem;
color: #222222;
background-color: #FFFFFF;
display: block;
font-family: ReithSans,Helvetica,Arial,sans-serif;
font-weight: 400;
font-style: normal;
font-size: 0.75rem;
line-height: 1rem;
color: #222222;
height: 2rem;
display: inline-block;
vertical-align: middle;
}
Expand All @@ -532,13 +458,6 @@ exports[`MediaIndicator should render video indicator correctly when inline 1`]
height: 100%;
}

@media (max-width:24.9375rem) {
.c0 {
height: 1.25rem;
padding: 0.25rem 0.25rem 0;
}
}

<div
aria-hidden="true"
class="c0"
Expand Down Expand Up @@ -573,16 +492,12 @@ exports[`MediaIndicator should render video indicator correctly when inline on R
}

.c0 {
padding: 0.5rem 0.25rem;
color: #222222;
background-color: #FFFFFF;
display: block;
font-family: "BBC Nassim Persian",Arial,Verdana,Geneva,Helvetica,sans-serif;
font-weight: 400;
font-style: normal;
font-size: 0.75rem;
line-height: 1rem;
color: #222222;
height: 2rem;
display: inline-block;
vertical-align: middle;
}
Expand All @@ -599,13 +514,6 @@ exports[`MediaIndicator should render video indicator correctly when inline on R
height: 100%;
}

@media (max-width:24.9375rem) {
.c0 {
height: 1.25rem;
padding: 0.25rem 0.25rem 0;
}
}

<div
aria-hidden="true"
class="c0"
Expand Down
24 changes: 4 additions & 20 deletions packages/components/psammead-media-indicator/src/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,40 +2,24 @@ import React from 'react';
import styled, { css } from 'styled-components';
import { string, oneOf, bool, node } from 'prop-types';
import { C_WHITE, C_EBON } from '@bbc/psammead-styles/colours';
import {
GEL_SPACING_HLF,
GEL_SPACING,
GEL_SPACING_QUAD,
} from '@bbc/gel-foundations/spacings';
import { GEL_GROUP_1_SCREEN_WIDTH_MAX } from '@bbc/gel-foundations/breakpoints';
import { GEL_MINION } from '@bbc/gel-foundations/typography';
import { GEL_SPACING_HLF } from '@bbc/gel-foundations/spacings';
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we still are using this GEL_SPACING_HLF,

When looking at where it's used, it's applied to the Time element
https://github.com/bbc/psammead/blob/302a5857b799b4f2cd192a0ccfe468ba5bd86487/packages/components/psammead-media-indicator/src/index.jsx#L33

Makes me think that this should be passed in as a Child, rather than be within this component.
We already have the children prop.

Copy link
Contributor

Choose a reason for hiding this comment

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

A consequence of the change would be that other components that use psammead-media-indicator would need to be updated.

I’ve looked at Psammead and Simorgh to see how much work this would be.
Psammead:
Stories and tests/test helpers in these two components, that can be carried out in one PR

packages/components/psammead-grid/src/index.stories.jsx:
packages/components/psammead-story-promo/src/index.stories.jsx:
packages/components/psammead-story-promo/src/index.test.jsx:
packages/components/psammead-story-promo/testHelpers/IndexAlsosContainer.jsx:

(Note, the top-level dependency in psammead would need to be updated)

In Simorgh, the changes would be to these containers - being carried out as part of the Desktop redesign: bbc/simorgh#5139

src/app/containers/StoryPromo/IndexAlsos/index.jsx:
src/app/containers/StoryPromo/MediaIndicator/index.jsx:

import { getMinion } from '@bbc/gel-foundations/typography';
import { getSansRegular } from '@bbc/psammead-styles/font-styles';
import { mediaIcons } from '@bbc/psammead-assets/svgs';

const MediaIndicatorWrapper = styled.div`
padding: ${GEL_SPACING} ${GEL_SPACING_HLF};
color: ${C_EBON};
background-color: ${C_WHITE};
display: block;
${({ service }) => getSansRegular(service)}
${GEL_MINION};
color: ${C_EBON};
height: ${GEL_SPACING_QUAD};
${({ script }) => script && getMinion(script)};

${({ isInline }) =>
isInline &&
css`
display: inline-block;
vertical-align: middle;
`}

${({ topStory }) =>
DenisHdz marked this conversation as resolved.
Show resolved Hide resolved
!topStory &&
css`
@media (max-width: ${GEL_GROUP_1_SCREEN_WIDTH_MAX}) {
height: 1.25rem;
padding: ${GEL_SPACING_HLF} ${GEL_SPACING_HLF} 0;
}
`}
`;

const FlexWrapper = styled.div`
Expand Down