-
Notifications
You must be signed in to change notification settings - Fork 78
[Consignments] Adds Storyboards and some typography helpers #548
Conversation
@@ -3,7 +3,7 @@ import { getStorybookUI, configure } from '@kadira/react-native-storybook'; | |||
|
|||
// import your stories | |||
configure(() => { | |||
require("../../src/storiesRoot") | |||
require("../../src/storiesRegistry") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
storiesRegistry felt like a better name for the place to add stories IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -8,6 +8,7 @@ | |||
"*.snap": "javascriptreact" | |||
}, | |||
"eslint.enable": false, | |||
"flow.enabled": false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this probably needs to go in most of our TS projects - I have a PR about this, but TBH I'm not sure if it's worth adding this at the extension level: flow/flow-for-vscode#95
"garamond-regular": "AGaramondPro-Regular", | ||
"garamond-italic": "AGaramondPro-Italic", | ||
"avant-garde-regular": "Avant Garde Gothic ITCW01Dm", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ATM all our font references are floating strings, this at least ties it together till TS 2.4.0
@@ -12,7 +12,7 @@ import NavigationButton from "../navigation_button" | |||
const smallButton = { height: 26, width: 320, marginBottom: 20 } | |||
const largeButton = { height: 26, width: 320, marginBottom: 20 } | |||
|
|||
storiesOf("Buttons") | |||
storiesOf("🎨 Buttons") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moves it to the top of the list :D
97a3740
to
6ae2287
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM! 👍🏻
fontFamily: fonts["garamond-regular"], | ||
}, | ||
|
||
subtitleDefault: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing a white space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - can't wait to get prettier on here so I can forget about this too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any lint checks in emission?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we have tslint, but it doesn't seem to have caught this: https://github.com/artsy/emission/blob/master/tslint.json
}) | ||
.add("App Serif Text", () => { | ||
return <Serif>This is a blank serif</Serif> | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you feel about doing .add("App Serif Text", () => <Serif>This is a blank serif</Serif>)
? Is it too much optimization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO - that's better, I'll get around to this on an upcoming PR :
}) | ||
.add("Info Page", () => { | ||
return <Info navigator={nav} route={route} /> | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
<Wrapper> | ||
<T.LargeHeadline>Large Headline</T.LargeHeadline> | ||
<T.SmallHeadline>Small Headline</T.SmallHeadline> | ||
<T.Subtitle>Subtitle</T.Subtitle> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice and interesting.
Builds on top of #543 - so, once that's merged I'll move this PR to point at
consignments
instead of the other PR.Adds typography setup, trying to get the general design down before writing specific views as it has it's own UI theme