Skip to content

Conversation

rybmat
Copy link
Contributor

@rybmat rybmat commented Apr 18, 2019

https://wikia-inc.atlassian.net/browse/IW-1945

Porting GlobalFooter component to react,

@Wikia/iwing

@rybmat rybmat requested a review from vforge as a code owner April 18, 2019 09:47
@rybmat
Copy link
Contributor Author

rybmat commented Apr 18, 2019

Tests are failing on master as well

vforge
vforge previously requested changes Apr 18, 2019
Copy link
Collaborator

@vforge vforge left a comment

Choose a reason for hiding this comment

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

I updated snapshots.

LGTM except one issue with it.


const Svg = ({ name, className, ...rest }) => (
<svg className={className} {...rest}>
<use xlinkHref={`#${name}`} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will not work unless the images have been added as a sprite - check http://localhost:6060/#!/GlobalFooter

You should use assets directly like so https://wikia.github.io/react-common/#logowikia :

import LogoWikia from '@wikia/react-common/source/assets/LogoWikia';

<LogoWikia height="80" />

Copy link
Collaborator

Choose a reason for hiding this comment

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

Demo page itself works because on the demo page ALL THE ASSETS are already included on the page.

See also https://github.com/Wikia/react-common/blob/master/source/components/GlobalNavigation/components/CommunityBar/CommunityBar.js#L16

xkxd added 3 commits April 19, 2019 14:06
# Conflicts:
#	docs/build/1.7afce2f4.js
#	docs/build/1.a4d7423d.js
#	docs/build/1.e08e0239.js
#	docs/build/bundle.faf2c36b.js
#	docs/index.html
@xkxd xkxd requested a review from vforge April 19, 2019 13:04
@xkxd xkxd dismissed vforge’s stale review April 23, 2019 07:33

all comments were fixed

@xkxd xkxd merged commit 8b75772 into master Apr 23, 2019
@xkxd xkxd deleted the IW-1945 branch April 23, 2019 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants