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

Remove unused offscreen text from psammead-media-indicator #1240

Merged
merged 16 commits into from
Jul 22, 2019
Merged

Remove unused offscreen text from psammead-media-indicator #1240

merged 16 commits into from
Jul 22, 2019

Conversation

Bopchy
Copy link
Contributor

@Bopchy Bopchy commented Jul 16, 2019

Resolves #1204

Overall change: Remove unused media indicator offscreen text.

Code changes:

  • Remove unused offscreen text from the MediaIndicator component.
  • Update tests.
  • Update stories.
  • Update snapshots.
  • I have assigned myself to this PR and the corresponding issues
  • Tests added for new features
  • Test engineer approval

@Bopchy Bopchy self-assigned this Jul 16, 2019
@Bopchy Bopchy changed the title 1204 drop off screen text Remove unused offscreen text from psammead-media-indicator Jul 16, 2019
@Bopchy Bopchy marked this pull request as ready for review July 16, 2019 12:39
@Bopchy Bopchy added the ws-home Tasks for the WS Home Team label Jul 16, 2019
This was referenced Jul 16, 2019
Closed
Closed
@Bopchy
Copy link
Contributor Author

Bopchy commented Jul 16, 2019

Created #1242 to update StoryPromo to use this version of MediaIndicator

andrew-nowak
andrew-nowak approved these changes Jul 16, 2019
View reviewed changes
Copy link
Contributor

@andrew-nowak andrew-nowak left a comment

Choose a reason for hiding this comment

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

deleting dead code :yay:

ghost
ghost suggested changes Jul 16, 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 version on package-lock.json.

ghost
ghost suggested changes Jul 17, 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.

Now that we don't use the offscreen text in media-indicator, we should remove psammead-visually-hidden-text from media-indicator dependancies.

andrew-nowak
andrew-nowak suggested changes Jul 17, 2019
View reviewed changes
Copy link
Contributor

@andrew-nowak andrew-nowak left a comment

Choose a reason for hiding this comment

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

looking again at this, the API is being changed so this is technically a breaking change and should be a major version bump

packages/components/psammead-media-indicator/CHANGELOG.md Outdated Show resolved Hide resolved
packages/components/psammead-media-indicator/package.json Outdated Show resolved Hide resolved
packages/components/psammead-media-indicator/package-lock.json Outdated Show resolved Hide resolved
dr3
dr3 reviewed Jul 17, 2019
View reviewed changes
lerna-debug.log Outdated Show resolved Hide resolved
dr3
dr3 reviewed Jul 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.

I think this should have been a minor change, as while the API has changed, its not breaking. Also theres a lerna debug file? TIL theyre a thing 🙈 Other than that, LGTM

j-pendlebury
j-pendlebury approved these changes Jul 18, 2019
View reviewed changes
Copy link
Contributor

@j-pendlebury j-pendlebury 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 to me 👍⭐️

@david-boydell
Copy link

Cool, this is ready for merge.

@Bopchy Bopchy merged commit 457b724 into latest Jul 22, 2019
@sareh sareh deleted the 1204-drop-off-screen-text branch August 7, 2019 10:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dr3 dr3 dr3 left review comments

@j-pendlebury j-pendlebury j-pendlebury approved these changes

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

@pjlee11 pjlee11 Awaiting requested review from pjlee11

@sareh sareh Awaiting requested review from sareh

Assignees

@Bopchy Bopchy

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.

media indicator: drop offscreenText
5 participants
@Bopchy @david-boydell @andrew-nowak @dr3 @j-pendlebury