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

Conversation

DenisHdz
Copy link
Contributor

@DenisHdz DenisHdz commented Feb 5, 2020

Resolves #NUMBER

Overall change:
After passing the inLine prop added in this PR to the Media Indicator used in the Index Alsos, we've realized that the MediaIndicatorWrapper styles are not suitable.

Before:
Screenshot 2020-02-05 at 10 00 26

After:
Screenshot 2020-02-05 at 09 59 51

For this reason, we've decided to remove the spacing on the Media Indicator so we can add them at a top-level in a separate PR (Story Promo/Index Alsos).

Code changes:

  • Remove padding and height from the MediaIndicatorWrapper
  • Remove the Time element from the Media Indicator
  • Update stories
  • Remove topStory test

  • 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

@DenisHdz DenisHdz added the ws-home Tasks for the WS Home Team label Feb 5, 2020
@DenisHdz DenisHdz self-assigned this Feb 5, 2020
} 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:

@sareh sareh mentioned this pull request Feb 5, 2020
6 tasks
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.

There is still a top story photogallery story even though topStory styles/prop got removed

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.

Everything looks good, inline toggle works properly now 👍

Copy link
Contributor

@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.

👍 Changes look good.
Looking at the ChromaticQA snapshots, there is a visual difference in the height for the 'inline' version of the MediaIndicator - this is expected.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ws-front-page-layout ws-home Tasks for the WS Home Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants