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

Move Media Icons from psammead-media-indicator into psammead-assets #2115

Merged
merged 16 commits into from
Sep 18, 2019
Merged

Move Media Icons from psammead-media-indicator into psammead-assets #2115

merged 16 commits into from
Sep 18, 2019

Conversation

OlgaLyubin
Copy link
Contributor

Resolves #1834

Overall change:
Moved Media Icons SVGs from psammead-media-indicator into psammead-assets so that they could be used by other components.

Code changes:
Moved media icons into psammead-assets.
Bumped up the version of psammead-assets and included them as dependencies of psammead-media-indicator.

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

@OlgaLyubin OlgaLyubin added ws-home Tasks for the WS Home Team shared-components ws-media The World Service media stream labels Sep 12, 2019
@OlgaLyubin OlgaLyubin self-assigned this Sep 12, 2019
ryanmccombe
ryanmccombe reviewed Sep 13, 2019
View reviewed changes
Copy link
Contributor

@ryanmccombe ryanmccombe left a comment

Choose a reason for hiding this comment

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

Needs package-lock.json, package.json and changelog.md update on both packages

Bopchy
Bopchy suggested changes Sep 13, 2019
View reviewed changes
Copy link
Contributor

@Bopchy Bopchy 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. Though as mentioned package.json, package-lock.json and CHANGELOG.md need to be updated for @bbc/psammead-media-indicator, and CHANGELOG.md needs to be updated for @bbc/psammead-assets

@OlgaLyubin
Copy link
Contributor Author

OlgaLyubin commented Sep 16, 2019 •
edited
Loading

I removed all of my changes for psammead-media-indicator and just left the work for psammead-assets . The reason for this is that before I can use media icons from psammead-assets, the new version of assets should be merged into Psammead. After this PR is merged I will create a new PR to pull in media SVGs into psammead-media-indicator .

EinsteinNjoroge
EinsteinNjoroge reviewed Sep 16, 2019
View reviewed changes
Copy link
Contributor

@EinsteinNjoroge EinsteinNjoroge left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

AlistairGempf
AlistairGempf approved these changes Sep 16, 2019
View reviewed changes
Copy link
Contributor

@AlistairGempf AlistairGempf 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 for the first step.

@OlgaLyubin
Copy link
Contributor Author

Shouldn't we remove the mediaIcons.jsx file from the psammead-media-indicator package as well?

So for now I am not removing anything from psammead-media-indicator because it is still used. When psammead-assets gets merged, then I will make changes to psammead-media-indicator including deleting the mediaIcons.jsx file.

dr3
dr3 approved these changes Sep 17, 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, can you raise an issue to add these media icons to storybook as isolated stories please :)

sareh
sareh suggested changes Sep 17, 2019
View reviewed changes
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.

Looks great. Just a point about naming consistently.

packages/utilities/psammead-assets/src/svgs/mediaIcons.jsx Outdated Show resolved Hide resolved
packages/utilities/psammead-assets/src/svgs.jsx Outdated Show resolved Hide resolved
This was referenced Sep 17, 2019
Closed
Closed
sareh
sareh approved these changes Sep 17, 2019
View reviewed changes
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.

👍 Thanks for the updates, @OlgaLyubin! Looks great.

@staylos92 staylos92 self-assigned this Sep 18, 2019
@staylos92
Copy link
Contributor

LGTM 👍

@OlgaLyubin OlgaLyubin merged commit c152d52 into latest Sep 18, 2019
@OlgaLyubin OlgaLyubin deleted the movemediaicons branch September 18, 2019 14:41
This was referenced Sep 19, 2019
Merged
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ryanmccombe ryanmccombe ryanmccombe left review comments

@sareh sareh sareh approved these changes

@AlistairGempf AlistairGempf AlistairGempf approved these changes

@EinsteinNjoroge EinsteinNjoroge EinsteinNjoroge approved these changes

@Bopchy Bopchy Bopchy approved these changes

@dr3 dr3 dr3 approved these changes

@jamesbhobbs jamesbhobbs Awaiting requested review from jamesbhobbs

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

@jroebu14 jroebu14 Awaiting requested review from jroebu14

Assignees

@staylos92 staylos92

@OlgaLyubin OlgaLyubin

Labels
shared-components ws-home Tasks for the WS Home Team ws-media The World Service media stream
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move Media Icons from psammead-media-indicator/mediaIcons into psammead-assets
9 participants
@OlgaLyubin @staylos92 @sareh @ryanmccombe @dr3 @Bopchy @EinsteinNjoroge @AlistairGempf @DenisHdz