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.

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

Code Review: Add support for Text Styles in JS API (#21903) #283

Merged
merged 24 commits into from Dec 11, 2018

Conversation

Projects
None yet
7 participants
@mathieudutour
Copy link
Contributor

mathieudutour commented Nov 13, 2018

fix BohemianCoding/Sketch#21903, fix #250, fix #217, fix #29, fix #269

TODO:

  • do we want to parse the fontName to show a fontWeight?
  • docs
  • deprecate old font methods on Text
@bohemian-coding-danger

This comment has been minimized.

Copy link
Collaborator

bohemian-coding-danger commented Nov 13, 2018

Warnings
⚠️

Source/dom/WrappedObject.js changed, but not:

  • 🧪 its tests

That's OK as long as you're refactoring.

⚠️

Source/dom/layers/Page.js changed, but not:

  • 📚 its docs
  • 🧪 its tests

That's OK as long as you're refactoring.

⚠️

Source/dom/models/Document.js changed, but not:

  • 📚 its docs
  • 🧪 its tests

That's OK as long as you're refactoring.

⚠️

Source/dom/models/SharedStyle.js changed, but not:

  • 🧪 its tests

That's OK as long as you're refactoring.

⚠️

Source/dom/style/Color.js changed, but not:

  • 📚 its docs

That's OK as long as you're refactoring.

⚠️

Source/dom/style/Style.js changed, but not:

  • 🧪 its tests

That's OK as long as you're refactoring.

Generated by 🚫 dangerJS

@robintindale

This comment has been minimized.

Copy link
Contributor

robintindale commented Nov 14, 2018

Could possibly be cool (but probably out of scope) to include CSS font weight like https://sketchplugins.com/d/193-how-to-get-font-weight/4

@mathieudutour

This comment has been minimized.

Copy link
Contributor

mathieudutour commented Nov 20, 2018

seems a bit out of scope indeed but you will only need this function:

function appKitWeightToCSSWeight(weight){
  return [100,100,100,200,300,400,500,500,600,700,800,900,900,900,900,900][weight]
}

I don't really want to expose it directly because there is less granularity in css so some info is lost in translation.

@pburtchaell

This comment has been minimized.

Copy link

pburtchaell commented Dec 4, 2018

Thank you! I am working on a plugin that needs this! Looking forward to the release. 😄

@bomberstudios
Copy link
Collaborator

bomberstudios left a comment

Made some comments, but I don't think any of those is a showstopper, so let's approve this 👍


const UNDERLINE_TRAIT = {
none: 0, // NSUnderlineStyleNone
solid: 0, // NSUnderlineStylePatternSolid

This comment has been minimized.

@bomberstudios

bomberstudios Dec 5, 2018

Collaborator

Is this correct? According to the previous line, 0 is none 🤔

This comment has been minimized.

@mathieudutour

mathieudutour Dec 5, 2018

Contributor

Weird, right? But that’s the enum I found in the macOS framework. I think it’s because it’s never just solid


if (hasTrait(underline, UNDERLINE_TRAIT['by-word'])) {
traits.push('by-word')
}

This comment has been minimized.

@bomberstudios

bomberstudios Dec 5, 2018

Collaborator

Why is solid not on this list?

This comment has been minimized.

@mathieudutour

mathieudutour Dec 5, 2018

Contributor

Because it’s always solid as soon as there is something else. So it just feels like boilerplate to include it

attribute = 1
} else if (_transform === 'lowercase') {
attribute = 2
} else if (_transform !== 'none' && transform) {

This comment has been minimized.

@bomberstudios

bomberstudios Dec 5, 2018

Collaborator

Not sure I'm reading this correctly: does this mean a plugin can set the textTransform attribute to anything?

This comment has been minimized.

@mathieudutour

mathieudutour Dec 5, 2018

Contributor

I’m usually doing this to support people that still use the native API: that way they can specify the native value of the transform (like 1 or 2 directly)

},
})

Style.define('fontStretch', {

This comment has been minimized.

@bomberstudios

bomberstudios Dec 5, 2018

Collaborator

Is this needed? AFAIK we're not exposing this in the UI…

This comment has been minimized.

@mathieudutour

mathieudutour Dec 5, 2018

Contributor

It’s a font variant, so it appears in the list of fonts (next to bold, italic, etc.)


text.style.lineHeight = 10
expect(text.style.lineHeight).toBe(10)
expect(text.style.paragraphSpacing).toBe(0)

This comment has been minimized.

@bomberstudios

bomberstudios Dec 5, 2018

Collaborator

Why are we checking this here?

This comment has been minimized.

@mathieudutour

mathieudutour Dec 5, 2018

Contributor

To make sure it doesn’t change


text.style.paragraphSpacing = 10
expect(text.style.paragraphSpacing).toBe(10)
expect(text.style.lineHeight).toBe(null)

This comment has been minimized.

@bomberstudios

bomberstudios Dec 5, 2018

Collaborator

Ditto for this one. Are we checking that changing one attribute doesn't change the other? If that's the case, maybe having a 'paragraph and line height should not influence each other' test to make this clearer?

This comment has been minimized.

@mathieudutour

mathieudutour Dec 5, 2018

Contributor

Could do that yes

expect(text.style.fontStyle).toBe(undefined)
})

// TODO: can't seem to find a font with small caps

This comment has been minimized.

@bomberstudios

bomberstudios Dec 5, 2018

Collaborator

In theory, 'system' should work, since San Francisco has native support for small caps

@pieteromvlee pieteromvlee merged commit ddb0392 into develop Dec 11, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@pieteromvlee pieteromvlee deleted the feature/21903 branch Dec 11, 2018

@kupe517

This comment has been minimized.

Copy link

kupe517 commented Dec 13, 2018

@mathieudutour When will these updates go to the production app? Can they be tested in the beta?

@mathieudutour

This comment has been minimized.

Copy link
Contributor

mathieudutour commented Dec 13, 2018

They will be in the Sketch 53 beta yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment