Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for textDecorationLine property #143

Merged
merged 3 commits into from
Jul 19, 2017
Merged

Add support for textDecorationLine property #143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a215c3
Add support for textDecoration property
Jul 8, 2017
6f4b5f1
Merge branch 'master' into textDecoration-feature
Jul 12, 2017
8dbfabd
Merge branch 'master' into textDecoration-feature
jemgold Jul 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/styling.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ Components use CSS styles + flexbox layout.
| `fontSize` | `number` | ✅ |
| `fontStyle` | `normal` | `italic` | ✅ |
| `fontWeight` | `string` | ✅ |
| `textDecorationLine` | `none` | `underline` | `double` | `line-through` | ✅ |
| `textShadowOffset` | `{ width: number, height: number }` | ⛔️ |
| `textShadowRadius` | `number` | ⛔️ |
| `textShadowColor` | `Color` | ⛔️ |
Expand Down
3 changes: 2 additions & 1 deletion src/buildTree.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,14 @@ const INHERITABLE_STYLES = [
'fontSize',
'fontStyle',
'fontWeight',
'textAlign',
'textDecoration',
Copy link
Contributor

Choose a reason for hiding this comment

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

we mirror the React Native APIs as much as possible — can you change this to textDecorationLine to match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Jon, I actually wanted to use the exact same API of textDecorationLine but I found this:
https://github.com/airbnb/react-sketchapp/blob/master/src/stylesheet/expandStyle.js#L61

When the style gets passed down as a textStyle this part expands it to be renamed textDecoration; I don't know why this is needed. I can go ahead and remove the shortHand here and add the textDecorationLine over here so it's propagated like it. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

🙃 I think maybe it was actually correct the first time - looks like I have your original commit and it's indeed working properly by expanding the textDecorationLine into textDecoration. Not sure what went wrong the first time I reviewed it, sorry!

Documentation update looks good - we want the public interface to be textDecorationLine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, then let me change it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Funny thing is that because of the expansion they both work textDecoration & textDecorationLine; Thoughts?

'textShadowOffset',
'textShadowRadius',
'textShadowColor',
'textTransform',
'letterSpacing',
'lineHeight',
'textAlign',
'writingDirection',
];

Expand Down
39 changes: 29 additions & 10 deletions src/jsonUtils/hacksForJSONImpl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,17 @@ export const TEXT_ALIGN = {
justify: TextAlignment.Justified,
};

export const TEXT_DECORATION_UNDERLINE = {
none: 0,
underline: 1,
double: 9,
};

export const TEXT_DECORATION_LINETHROUGH = {
none: 0,
'line-through': 1,
};

// this doesn't exist in constants
export const TEXT_TRANSFORM = {
uppercase: 1,
Expand Down Expand Up @@ -72,7 +83,8 @@ export const makeImageDataFromUrl = (url: string): MSImageData => {
let image;

if (!fetchedData) {
const errorUrl = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mM8w8DwHwAEOQHNmnaaOAAAAABJRU5ErkJggg==';
const errorUrl =
'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mM8w8DwHwAEOQHNmnaaOAAAAABJRU5ErkJggg==';
image = NSImage.alloc().initWithContentsOfURL(
NSURL.URLWithString(errorUrl)
);
Expand All @@ -96,20 +108,28 @@ export function makeAttributedString(string: ?string, textStyle: TextStyle) {
color.red,
color.green,
color.blue,
color.alpha,
color.alpha
),
NSUnderline: TEXT_DECORATION_UNDERLINE[textStyle.textDecoration] || 0,
NSStrikethrough: TEXT_DECORATION_LINETHROUGH[textStyle.textDecoration] || 0,
};

if (textStyle.letterSpacing !== undefined) {
attribs.NSKern = textStyle.letterSpacing;
}

if (textStyle.textTransform !== undefined) {
attribs.MSAttributedStringTextTransformAttribute = TEXT_TRANSFORM[textStyle.textTransform] * 1;
attribs.MSAttributedStringTextTransformAttribute =
TEXT_TRANSFORM[textStyle.textTransform] * 1;
}

const attribStr = NSAttributedString.attributedStringWithString_attributes_(string, attribs);
const msAttribStr = MSAttributedString.alloc().initWithAttributedString(attribStr);
const attribStr = NSAttributedString.attributedStringWithString_attributes_(
string,
attribs
);
const msAttribStr = MSAttributedString.alloc().initWithAttributedString(
attribStr
);

return encodeSketchJSON(msAttribStr);
}
Expand All @@ -130,14 +150,13 @@ export function makeTextStyle(textStyle: TextStyle) {
color.red,
color.green,
color.blue,
color.alpha,
),
color.alpha
)
),
NSParagraphStyle: encodeSketchJSON(pStyle),
NSKern: textStyle.letterSpacing || 0,
MSAttributedStringTextTransformAttribute: TEXT_TRANSFORM[
textStyle.textTransform || 'initial'
] * 1,
MSAttributedStringTextTransformAttribute:
TEXT_TRANSFORM[textStyle.textTransform || 'initial'] * 1,
},
};

Expand Down