Feat/video label #704
Merged
Feat/video label #704
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nikhedonia
force-pushed
the
feat/video-label
branch
6 times, most recently
from
fa69f3d
to
1ddb933
Compare
nikhedonia
force-pushed
the
feat/video-label
branch
from
e499664
to
e890977
Compare
colincclark
suggested changes
Mar 7, 2018
| import renderer from "react-test-renderer"; | ||
| import VideoLabel from "../video-label"; | ||
|
|
||
| module.exports = () => { |
|
|
||
| const styles = { | ||
| title: { | ||
| ...sharedStyles.title |
There was a problem hiding this comment.
these index files do not seem to add anything...
| <Text style={[styles.title, { color, marginLeft: 5 }]}> | ||
| {beautifyTitle("VIDEO")} | ||
| </Text> | ||
| {title ? ( |
There was a problem hiding this comment.
&& instead of a ternary?
There was a problem hiding this comment.
to my surprise this does not seem to work:
Unexpected text node: . A text node cannot be a child of a <View>.
| {title ? ( | ||
| <Text style={[styles.title, { color, marginLeft: 3 }]}>|</Text> | ||
| ) : null} | ||
| {title ? ( |
| .add("VideoLabel with title", () => ( | ||
| <VideoLabel | ||
| title="SWIMMING" | ||
| color={select( |
aronshamash
suggested changes
Mar 7, 2018
| // apply transformations to add uppercase and letter spacing. | ||
| // letterSpacing CSS prop does not work on android: | ||
| // https://github.com/facebook/react-native/pull/13199 | ||
| const beautifyTitle = title => |
There was a problem hiding this comment.
maybe we can now get rid of this letter spacing hack in light of facebook/react-native#17398. Just need to check
There was a problem hiding this comment.
Not part of react-native-0.53.3
colincclark
approved these changes
Mar 7, 2018
aronshamash
reviewed
Mar 7, 2018
packages/video-label/video-label.js
Outdated
| {title ? ( | ||
| <Text style={[styles.title, { color, paddingLeft: 3 }]}>{title}</Text> | ||
| <Text style={[styles.title, { color, paddingLeft: 5 }]}>{title.toUpperCase()}</Text> |
aronshamash
reviewed
Mar 7, 2018
packages/icons/icons/video.js
Outdated
| @@ -6,8 +6,9 @@ import iconPropTypes from "./proptypes"; | |||
| const viewBox = | |||
| "0.15463916957378387 0.049614034593105316 23.59917640686035 13.728596687316895"; | |||
|
|
|||
| const ratio = 1.72; | |||
There was a problem hiding this comment.
In twitter.js we using a ratio here we using a decimal. Can use 43 / 25 for video and 75 / 60 for twitter. Other than this it seems good to merge
nikhedonia
force-pushed
the
feat/video-label
branch
from
0acfd7d
to
6ec45c3
Compare
aronshamash
approved these changes
Mar 7, 2018
nikhedonia
force-pushed
the
feat/video-label
branch
from
fcc7fea
to
7f3028b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements video label as a seperate package.
In order to avoid magic numbers when using the video-icon inside the video label, I made width of icons optional and compute the width from the aspect ratio and height.
reference: https://www.thetimes.co.uk/past-six-days/2018-03-06/sport/weekend-review-mourinhos-tumble-richarlisons-tantrum-and-dioufs-spat-tfsfwtbkf