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

Dynamic brand link #528

Merged
46 commits merged into from
May 22, 2019
Merged

Dynamic brand link #528

46 commits merged into from
May 22, 2019

Conversation

ghost
Copy link

@ghost ghost commented May 16, 2019

Resolves #500

Overall change: Make Branding href link dynamic to allow passing of different service links.

Code changes:

  • Added a link prop which is required to allow passing of the service url to brand
  • Handled what should happen incase a link is not provided to Brand
  • Added a story to preview Brand with link not provided.

  • I have assigned myself to this PR and the corresponding issues
  • Tests added for new features
  • Test engineer approval

@ghost ghost added the ws-home Tasks for the WS Home Team label May 16, 2019
@ghost ghost self-assigned this May 16, 2019
@ghost ghost marked this pull request as ready for review May 16, 2019 07:05
DenisHdz
DenisHdz previously approved these changes May 17, 2019
Copy link
Contributor

@DenisHdz DenisHdz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@12 12 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! Got a couple of comments.

packages/components/psammead-brand/CHANGELOG.md Outdated Show resolved Hide resolved
packages/components/psammead-brand/README.md Outdated Show resolved Hide resolved
packages/components/psammead-brand/src/index.stories.jsx Outdated Show resolved Hide resolved
packages/components/psammead-brand/src/index.jsx Outdated Show resolved Hide resolved
dr3
dr3 previously approved these changes May 21, 2019
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, nice work

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.

Looks mostly good to me, just some NaNrems appearing in the css output, which should probably be avoided.

@ghost ghost dismissed dr3’s stale review via 6d8e5a1 May 21, 2019 10:34
@jamesbrumpton
Copy link
Contributor

LGTM. Happy for this to be merged.

@ghost ghost merged commit b84fb3b into latest May 22, 2019
@ghost ghost deleted the dynamic-brand-link branch May 22, 2019 09:38
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ws-home Tasks for the WS Home Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Branding SVGs link href dynamic
7 participants