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.

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

Colors #345

Merged
merged 23 commits into from Jan 28, 2019

Conversation

Projects
None yet
4 participants
@WCByrne
Copy link
Contributor

WCByrne commented Jan 17, 2019

Issue: #44

Adds API access to document and global color/gradient assets using new ColorAsset and GradientAsset classes.

Global

This is only read at the moment

sketch.globalColors() // [ColorAsset]
sketch.globalGradients() // [GradientAsset]

Document

let colors = doc.colors // [ColorAsset]
let gradients = doc.gradients // [GradientAsset]

colors[0].color // hex
gradients[0].gradient // Gradient

 // Push a color as a hex string or native object (i.e NSColor, MSColor, MSColorAsset, etc)
// Name defaults to null
colors.push('#AABBCCFF')

// Push a gradient as dictionary or existing Gradient object. 
gradients.push({
  gradientType: 'Linear',
  ...
})


// Optionally include a name
colors.push({
  name: 'My Color',
  color: '#112233', // Or native
})
gradients.push({
  name: 'My Gradient',
  gradient: {}
})

TODO

  • Should you be able to set global colors/gradients?
@bohemian-coding-danger

This comment has been minimized.

Copy link
Collaborator

bohemian-coding-danger commented Jan 17, 2019

Warnings
⚠️ This pull request may need a CHANGELOG entry.
⚠️

Changes were made to package.json, but not to package-lock.json - Perhaps you need to run npm run install?

⚠️

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

  • 🧪 its tests

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

⚠️

Source/dom/assets/Asset.js changed, but not:

  • 🧪 its tests

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

⚠️

Source/dom/assets/ColorAsset.js changed, but not:

  • 📚 its docs

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

⚠️

Source/dom/assets/GradientAsset.js changed, but not:

  • 📚 its docs

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

⚠️

Source/dom/assets/index.js changed, but not:

  • 📚 its docs
  • 🧪 its tests

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

⚠️

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

  • 📚 its docs

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

Generated by 🚫 dangerJS

WCByrne added some commits Jan 19, 2019

@WCByrne

This comment has been minimized.

Copy link
Contributor Author

WCByrne commented Jan 21, 2019

Ready for another review here. I considered breaking the new classes out into separate files but it seemed like extra clutter - happy to do so if that’s better.

WCByrne added some commits Jan 22, 2019

Rename ColorAsset Asset and improve tests
Allow ColorAsset from MSColor

@WCByrne WCByrne force-pushed the WCByrne:colors branch from 9b87b37 to 83265fc Jan 22, 2019

@WCByrne WCByrne force-pushed the WCByrne:colors branch from 83265fc to d808252 Jan 22, 2019

Show resolved Hide resolved Source/dom/models/Asset.js Outdated
Show resolved Hide resolved Source/dom/models/Asset.js Outdated
Show resolved Hide resolved Source/dom/models/Asset.js Outdated
Show resolved Hide resolved Source/dom/models/Asset.js Outdated
Show resolved Hide resolved Source/dom/models/Asset.js Outdated
Show resolved Hide resolved docs/api/Document.md Outdated
Show resolved Hide resolved Source/dom/models/Asset.js Outdated

mathieudutour and others added some commits Jan 23, 2019

Update Source/dom/models/Asset.js
Use Asset keys and delete id from subclasses

Co-Authored-By: WCByrne <wesburn@me.com>
Apply suggestions from code review
Use Asset as base for defined properties

Co-Authored-By: WCByrne <wesburn@me.com>
Show resolved Hide resolved Source/dom/index.js Outdated
@WCByrne

This comment has been minimized.

Copy link
Contributor Author

WCByrne commented Jan 28, 2019

Let me know if there is anything else to fix/improve here

@mathieudutour
Copy link
Contributor

mathieudutour left a comment

I think we are nearly there! Just a few documentation updates which I can take care of if you want

@@ -15,6 +15,8 @@ A Sketch document.
| id<span class="arg-type">string</span> | The unique ID of the document. |
| pages<span class="arg-type">[Page](#page)[]</span> | The pages of the document. |
| path<span class="arg-type">string</span> | The path to the document (or the appcast URL in case of a Document from a remote Library. |
| colors<span class="arg-type">[GraientAsset]</span> | A list of color assets defined in the document. Mutating the returned array will update the document colors. |

This comment has been minimized.

Copy link
@mathieudutour

mathieudutour Jan 28, 2019

Contributor

The documentation for the asset is missing

This comment has been minimized.

Copy link
@WCByrne

WCByrne Jan 28, 2019

Author Contributor

I wasn't sure where it should go

This comment has been minimized.

Copy link
@WCByrne

WCByrne Jan 28, 2019

Author Contributor

Same with the global getters

This comment has been minimized.

Copy link
@mathieudutour

mathieudutour Jan 28, 2019

Contributor

So I'd create a new file in the Models section called Asset. Then each asset is a ## header and the global getter a ### under it.

What do you think?

This comment has been minimized.

Copy link
@WCByrne

WCByrne Jan 28, 2019

Author Contributor

Seems like it makes sense. I haven't worked with the docs enough yet to picture what that would actually look like. I can try it.

It's not totally clear in the Readme if I can build and view the docs locally. Is that possible?

This comment has been minimized.

Copy link
@mathieudutour

mathieudutour Jan 28, 2019

Contributor

yeah, npm run docs:start

This comment has been minimized.

Copy link
@WCByrne

WCByrne Jan 28, 2019

Author Contributor

Ok, added an Assets section to the docs.

Show resolved Hide resolved Source/dom/globalAssets.js Outdated
*
* @return {Array<ColorAsset>} A list of color assets defined globally
*/
export function globalColors() {

This comment has been minimized.

Copy link
@mathieudutour

mathieudutour Jan 28, 2019

Contributor

The documentation for this is missing

This comment has been minimized.

Copy link
@WCByrne

WCByrne Jan 28, 2019

Author Contributor

Again, I wasn't sure where it should go

WCByrne added some commits Jan 28, 2019

mathieudutour added some commits Jan 28, 2019

@mathieudutour
Copy link
Contributor

mathieudutour left a comment

👏

@mathieudutour mathieudutour merged commit cea7be5 into BohemianCoding:develop Jan 28, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@WCByrne

This comment has been minimized.

Copy link
Contributor Author

WCByrne commented Jan 30, 2019

@mathieudutour is this going to make it into 53?

@mathieudutour

This comment has been minimized.

Copy link
Contributor

mathieudutour commented Jan 30, 2019

no, it needs to go through QA and we are releasing next week so we won't have time. maybe 53.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.