-
Notifications
You must be signed in to change notification settings - Fork 818
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
Create getting started docs page #495
Create getting started docs page #495
Conversation
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! Just a few suggestions
docs/guides/getting-started.md
Outdated
npm run render | ||
``` | ||
|
||
If you forget to create a new document, you may get an error saying `null is not an object`. You can handle this case with the instructions below: |
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.
not to be addressed in this PR but it'd be nice to throw a better error message
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.
👍 Is it not worth creating a new Sketch document by default in the render function? If no documents are found.
e.g.
react-sketchapp/src/utils/getDocument.ts
Lines 3 to 8 in 9ed5580
export const getDocumentDataFromContext = (ctx: SketchContext): SketchDocumentData => | |
( | |
ctx.document || | |
(ctx.actionContext || {}).document || | |
NSDocumentController.sharedDocumentController().currentDocument() | |
).documentData(); |
->
export const getDocumentDataFromContext = (ctx: SketchContext): SketchDocumentData => {
try {
return (
ctx.document ||
(ctx.actionContext || {}).document ||
NSDocumentController.sharedDocumentController().currentDocument()
).documentData();
} catch(err) {
if (sketch.documents.length === 0) {
return new sketch.Document().sketchObject.documentData();
}
}
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.
No I don't think it should do something that it wasn't explicitly told not to. If we have a nice error message with some suggestion to fix the issue, it will be a lot more actionable than if we sometimes create a new doc but not always
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.
Should I open up a PR for:
export const getDocumentDataFromContext = (ctx: SketchContext): SketchDocumentData => {
try {
return (
ctx.document ||
(ctx.actionContext || {}).document ||
NSDocumentController.sharedDocumentController().currentDocument()
).documentData();
} catch(err) {
if (err.includes('null is not an object')) {
throw new Error('No open document found. Please try opening/creating a document, or you could use \'render(<App />, sketch.getSelectedDocument() || new sketch.Document())\'');
}
}
I'd use if (sketch.getDocuments().length === 0) { throw ... }
, but not sure if it's best to avoid the public Sketch API? Couldn't find any imports for it in the source.
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.
Let's avoid the plugin sketch API for now (because it might be a pain on nodejs and not working on sketch <49).
But I think it can be just
export const getDocumentDataFromContext = (ctx: SketchContext): SketchDocumentData => {
let document = ctx.document ||
(ctx.actionContext || {}).document ||
NSDocumentController.sharedDocumentController().currentDocument()
if (!document || !document.documentData) {
throw new Error('No open document found.\nTry opening/creating a document before calling `render`.\n\nFor more information, check out http://airbnb.io/react-sketchapp/docs/guides/rendering.html');
}
return document.documentData()
}
docs/guides/styling.md
Outdated
@@ -105,6 +105,7 @@ Some properties are Sketch specific and won't work cross-platform but give you a | |||
| --- | --- | --- | | |||
| `shadowSpread` | `number` | ✅ | | |||
| `shadowInner` | `boolean` | ✅ | | |||
| `textTransform` | `none` | `uppercase` | `lowercase` | ✅ | |
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.
let's put it in Type Style
and open an issue for the support of capitalize
docs/guides/getting-started.md
Outdated
|
||
If you forget to create a new document, you may get an error saying `null is not an object`. You can handle this case with the instructions below: | ||
|
||
### Rendering to Multiple Pages or New Documents |
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.
I think that could be in a guide of their own - maybe called Rendering
or something
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.
Created rendering.md
Co-Authored-By: Mathieu Dutour <[email protected]>
Co-Authored-By: Mathieu Dutour <[email protected]>
Co-Authored-By: Mathieu Dutour <[email protected]>
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.
perfect, thanks!
Hopefully addresses #263 (there's overlap with the slow start PR, but decided to carry on the effort as a more general purpose getting started page, as it's been inactive).
textTransform
to docs (included in "React Sketch.app only" section, ascapitalize
isn't supported, unlike RN and web) fixes Document textTransform support #491