Skip to content
This repository was archived by the owner on Feb 23, 2021. It is now read-only.

Conversation

@valentinewallace
Copy link
Contributor

@valentinewallace valentinewallace commented Jun 8, 2018

@tanx what do you think about using these for our purple spinners? they can't do the gradient from the invision project but they should be able to scale to the correct size

Closes #279

@tanx
Copy link
Contributor

tanx commented Jun 11, 2018

@tanx what do you think about using these for our purple spinners? they can't do the gradient from the invision project but they should be able to scale to the correct size

Mmmh. Probably good enough for a first iteration. Let me test it out.

Copy link
Contributor

@tanx tanx left a comment

Choose a reason for hiding this comment

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

I made a few minor changes. Can you get it to scale?

.addDecorator(story => (
<MainContent style={{ justifyContent: 'center' }}>{story()}</MainContent>
))
.add('SmallSpinner', () => <SmallSpinner />);
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to set height/width via the style attribute but I couldn't scale it 🤔

@valentinewallace
Copy link
Contributor Author

valentinewallace commented Jun 11, 2018

Added scaling :)

I was thinking of making the percentage-based medium spinners using this tutorial: https://cmichel.io/react-native-progress-circle LMK if you think that's not a good idea

@valentinewallace valentinewallace force-pushed the spinner-components branch 2 times, most recently from db42cc6 to 8b9bedd Compare June 14, 2018 08:05
@valentinewallace
Copy link
Contributor Author

valentinewallace commented Jun 14, 2018

@tanx Hey, using the ART library I couldn't get the linear gradient to work. are you opposed to using this library? https://github.com/godaddy/svgs

@tanx tanx force-pushed the spinner-components branch from 8b9bedd to 932cfb6 Compare June 19, 2018 11:07
Copy link
Contributor

@tanx tanx left a comment

Choose a reason for hiding this comment

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

I was thinking of making the percentage-based medium spinners using this tutorial: https://cmichel.io/react-native-progress-circle LMK if you think that's not a good idea

That looks really promising.

@tanx Hey, using the ART library I couldn't get the linear gradient to work. are you opposed to using this library? https://github.com/godaddy/svgs

Sounds good. Looks like we'll need svgs anyway for this #289. I sent a PR to create-react-app a while back to add support for svgs facebook/create-react-app#4224

I also rebased this branch on the current master and added some comments in my review.

endDegree >= 180
);
return p;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. Could we begin the circle from to top/center like in the design?
image

StyleSheet,
View,
ART,
} from 'react-native';
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like there are currently linter errors in the build https://travis-ci.org/lightninglabs/lightning-app/jobs/394053652#L4509

@valentinewallace valentinewallace force-pushed the spinner-components branch 2 times, most recently from b40dc20 to 67bc896 Compare June 21, 2018 00:43
@valentinewallace
Copy link
Contributor Author

@tanx got the gradient working! still working on how to modularize the components to make them reusable for the remaining spinners

Copy link
Contributor

@tanx tanx left a comment

Choose a reason for hiding this comment

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

Cool. Haven’t tested it yet. Just one comment so far...

import { color, font } from './style';
import Icon from '../component/icon';
import Text from '../component/text';
import Svg, { Path, Circle, Defs, Stop, LinearGradient } from 'svgs';
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that svgs is web only, I’d wrap it and re-export it in an svg.js component. That way we can implement a svg.ios.js and svg.android.js components that re-export react-native-svg. See https://facebook.github.io/react-native/docs/platform-specific-code.html

Copy link
Contributor

@tanx tanx left a comment

Choose a reason for hiding this comment

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

Added a few comments

"react-native-web": "^0.6.0",
"react-scripts": "^1.1.1"
"react-scripts": "^1.1.1",
"svgs": "^3.2.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the package-lock.json shouldn’t change so much here. Which node/npm version are you using? We should stick with the current LTS node@8.x/npm@5.x since newer versions of npm will rebuild the package-lock.json and potentially cause incompapatibilites on travis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, i'll switch versions

Defs,
Stop,
LinearGradient,
} from '../component/svg';
Copy link
Contributor

Choose a reason for hiding this comment

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

./svg from the components dir (for text/icon too)

export const LinearGradient = LinearGradientJS;

const Svg = SvgJS;
export default Svg;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should re-export all here. Try

export * from 'svgs';

See https://stackoverflow.com/questions/29844074/es6-export-all-values-from-object

@tanx tanx force-pushed the spinner-components branch from 51f9ff5 to 607dad0 Compare June 22, 2018 13:43
"prop-types": "15.6.1",
"rip-out": "1.0.0"
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

@valentinewallace I removed the changes and rebased the branch.

@tanx
Copy link
Contributor

tanx commented Jun 22, 2018

Also, I was getting a warning in the cobsole in storybook.

@valentinewallace
Copy link
Contributor Author

Also, I was getting a warning in the cobsole in storybook.

working on it... can't seem to properly import resize-observer-polyfill

@valentinewallace valentinewallace self-assigned this Jun 25, 2018
@tanx
Copy link
Contributor

tanx commented Jun 25, 2018

working on it... can't seem to properly import resize-observer-polyfill

Ok cool.

@valentinewallace
Copy link
Contributor Author

Plan is to move the LoadNetworkSpinner into the file for this screen once this PR is finalized, since it's custom-made for that screen

});
export const SmallSpinner = ({ ...props }) => (
<ActivityIndicator
sizeM="small"
Copy link
Contributor

Choose a reason for hiding this comment

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

@valentinewallace
Copy link
Contributor Author

@tanx could we close this in favor of #406? it includes the same code but rebasing on top would be annoying because i moved the LoadNetworkSpinner into the file specific to its screen

Copy link
Contributor

@tanx tanx left a comment

Choose a reason for hiding this comment

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

Looks really good 👍 I made some minor resize changes and have just a small comment on the storybook entry.

<Background
color={color.blackDark}
style={{ flexDirection: 'row', width: '100%' }}
>
Copy link
Contributor

Choose a reason for hiding this comment

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

You're wrapping a Background inside of a MainContent. They're meant to be used the other way around. You wouldn't need to specify the width in that case.

Copy link
Contributor

@tanx tanx left a comment

Choose a reason for hiding this comment

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

I made a few more minor changes. But looks good now.

@tanx tanx merged commit 2ae5b54 into master Jul 3, 2018
@tanx tanx deleted the spinner-components branch July 3, 2018 09:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants