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

Make media indicator shorter under 400px #776

Merged
merged 18 commits into from
Jul 9, 2019
Merged

Make media indicator shorter under 400px #776

merged 18 commits into from
Jul 9, 2019

Conversation

DenisHdz
Copy link
Contributor

@DenisHdz DenisHdz commented Jul 4, 2019 •
edited
Loading

Resolves #768

Overall change:
Change Media indicator padding under 400px when not a Top Story.

Code changes:

  • Pass topStory prop to the MediaIndicatorWrapper.
  • Change MediaIndicatorWrapper padding-top to 4px and padding-bottom to 0px below 400px , when not a top story.
  • Update snapshots
  • I have assigned myself to this PR and the corresponding issues
  • Tests added for new features
  • Test engineer approval

@DenisHdz DenisHdz added the ws-home Tasks for the WS Home Team label Jul 4, 2019
@DenisHdz DenisHdz self-assigned this Jul 4, 2019
ghost
ghost suggested changes Jul 5, 2019
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Update README.md

dr3
dr3 approved these changes Jul 5, 2019
View reviewed changes
Copy link
Contributor

@dr3 dr3 left a comment

Choose a reason for hiding this comment

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

LGTM, could be a minor bump, rather than patch IMO, but not too fussed

@david-boydell
Copy link

Hi @DenisBBC,

I've had a look at this sub-400px, I'm still seeing the padding present. I'm on origin/media-indicator-padding and this is what I see:

image

If I've misunderstood this one let me know.

@dr3
Copy link
Contributor

dr3 commented Jul 8, 2019 •
edited
Loading

I've had a look at this sub-400px, I'm still seeing the padding present. I'm on origin/media-indicator-padding and this is what I see:

@david-boydell What commands are your running?

@david-boydell
Copy link

@dr3,

I've pulled latest, switched to the branch and then run npm run ci:packages && npm run storybook

@dr3
Copy link
Contributor

dr3 commented Jul 8, 2019

@david-boydell Your looking at story-promo when the changes are in media-indicator. The story promo story uses the changes on latest. You can try npm run install:packages:link, but your best bet would be cp -r ../psammead/packages/components/psammead-media-indicator/dist/ ./node_modules/@bbc/psammead-media-indicator/dist in simorgh and looking at it in simorgh

@andrew-nowak andrew-nowak self-assigned this Jul 8, 2019
@andrew-nowak
Merge remote-tracking branch 'origin/latest' into media-indicator-pad…
9b24a6b 
…ding

# Conflicts:
#	packages/components/psammead-media-indicator/CHANGELOG.md
#	packages/components/psammead-media-indicator/package-lock.json
#	packages/components/psammead-media-indicator/package.json
@andrew-nowak
snapshots
ad36374
@andrew-nowak
Merge remote-tracking branch 'origin/latest' into media-indicator-pad…
0b79522 
…ding
@andrew-nowak
even more snapshots
fb001ec
@david-boydell
Copy link

Have verified on the MediaIndicator component. This is ready for merge.

@dr3 dr3 merged commit 30ea37d into latest Jul 9, 2019
@dr3 dr3 deleted the media-indicator-padding branch July 9, 2019 08:05
@jroebu14 jroebu14 mentioned this pull request Jul 10, 2019
Closed
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dr3 dr3 dr3 approved these changes

@andrew-nowak andrew-nowak andrew-nowak approved these changes

@j-pendlebury j-pendlebury Awaiting requested review from j-pendlebury

@pjlee11 pjlee11 Awaiting requested review from pjlee11

@sareh sareh Awaiting requested review from sareh

Assignees

@andrew-nowak andrew-nowak

@DenisHdz DenisHdz

Labels
ws-home Tasks for the WS Home Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make media indicator shorter @ <400px
4 participants
@DenisHdz @david-boydell @dr3 @andrew-nowak