sketch-hq / sketch-assistants Goto Github PK

• Feature should be configurable/optional, i.e. no limits in Node, limited in Sketch
• Assistant result should indicate whether its complete, or has been artificially limited. If it's been limited we won't be able to tell decisively whether the Assistant has "passed" or not, which may have UI implications - e.g. show when a result is indeterminate, explain why, and what to do about it.

Timeout

Introducing a timeout could lead results to be somewhat non deterministic, e.g. number of violations returned could vary depending on CPU load and how fast the rule executed. However it would be a good way to ensure that Assistant run time doesn't get totally out of control.

Considering that most rules should be capable of doing their thing quite efficiently, perhaps a 100ms timeout per rule could be considered? That would still be long enough for a rule to return 100s or even 1000s of violations depending on the document.

The downside would be rules that need to take a long time to run - could be annoying if they just timeout immediately before returning any results. But we could fine-tune this, or even eventually make it configurable in preferences.

Question - is the timeout per rule, per Assistant or per run? If each rule gets 100ms, but there are a ton of Assistants added to the document, with lots of rules each, then runtime could still start to get a bit long. Alternatively we could assign a time budget for the entire run, like max 4000ms, with each rule getting an equal slice of that.

Violation budget

The downside of a violation budget is it doesn't help with the case of rules that take an overly long time to run. If there's a violation budget of 200 per rule, but a rule is inefficient, then the Assistant runtime could still be way too long.

Also with efficiency improvements in the Sketch UI, then rendering large sets of violations doesn't seem to be an issue any more. So an efficient rule generating a good amount of violations before it timed-out doesn't seem like a problem any longer.

Thoughts @christianklotz @craig006

Related to rules:

• layer-styles-prefer-library
• text-styles-prefer-library
• symbols-prefer-library

Changes to reduce scope and fix:

• Rename to the format library-*-allowed-libraries
• Only check objects that are already using a library resource, ignore objects using local resources
• Do not raise violations if libraries option is empty [] (the meaning of an empty option array is now ambiguous in the context of these reworked rules)
• Group layers together that are all using the same unauthorized library into one violation, where possible
• Fix: handle library resources used in overrides

Merges #55 to release 5.0.0-next.15 of the monorepo. This includes the first release of the cli package.

QA

Build the three Assistants locally from master (i.e. package as tarballs), and check they work with the current Sketch Private release.

If they work as expected, then we are good to make the release (i.e. won't be breaking things for our testers).

At the moment cache iteration is achieved via a callback based approach:

utils.iterateCache({
  textLayer(node: Node) {
    // Perform rule logic
  }
})

This is all well and good, but it's potentially overly-complex and quite inflexible. If instead we exposed standard iterable objects for the various cache items then it enables a host of flexible approaches using native language features out of the box

• for ... of loops
• Await promises in the loops
• Convert to arrays with Array.from
• Using array spread syntax
• And more

Additionally if the iterable objects were properly typed we could avoid some of the type casting gymnastics we're doing at the moment in rules. i.e. casting from a generic Node to a more specific type like Node<FileFormat.TextLayer>.

Proposed syntax to loop a cache item

for (const textLayer of utils.iterators.textLayers) {
  // textLayer is typed as Node<FileFormat.TextLayer>
  // Perform rule logic
}

Once the new API is available rule utils, then we can transition the core rules to make use of it, and in due course remove the old iteration api.

This is causing a rule error with the latest ignore code.

Best to update this rule and release new Assistants once the ignore feature has made its way to Sketch Private.

https://github.com/sketch-hq/sketch-assistants/blob/main/assistants/core/src/rules/groups-max-layers/index.ts#L45

One option could be to adjust the value, not sure how while maintaining one, though.

Groups should have less than #+1 layers

A quicker fix might be:

Groups should have no more than # layers

The Sketch Naming Conventions Assistant requires artboards to start with numbers and spaces but the copy doesn't make it explicitly clear that the space is required, too:

Instead of:

Artboard names should start with numbers

… let's change it to:

Artboard names should start with numbers followed by a space

Test helpers:

• Test a single rule in isolation with custom config
• Test a single rule within the context of an Assistant
• Test an entire Assistant

QA

Not required. This only touches test code, and doesn't require a release. If the tests go green we're good to merge.

Add remaining Assistant-related packages to the monorepo:

• Types
• Utils
• Core Rules Assistant

When using Abstract, library names change all the time - depending on the branch you are currently working in.
It would be helpful to additionally allow library ids as values for whitelisted libraries and not only library names.

This is the temporary library name of the library "G-Basic" in the Abstract branch “SketchAssistants improvements” when opening a file in Sketch through Abstract: "G-Basics (SketchAssistants improvements)".

Icons tend not to place layers on 0.0 and 0.5 pixel increments.

It gets tedious clicking warnings one by one (a single icon might have 10 such layers) for standard fontawesome icons etc.

Imagine how wonderful it would be if the assistant didn't complain when the symbol artboard contained a keyword like #allowsubpixels

User got very confused by the “Symbols should be used” rule.

https://www.notion.so/sketchapp/29-April-1-May-f9630b3339ff4d14895de447044af47d#cdeb615f5d0d40eaad34319bab12509a

UPDATE:
After some discussion it was determined that this rule is too strict and not very useful, so it's best to simply remove it from the assistant for now.

Summary
I'm being told by the "Tidy" assistants I should use symbols, on artboards that are symbols.

Steps to Reproduce
Steps required to reproduce the problem:

1. Opened comments-annotations.sketch.zip
2. Ran the "Tidy" assistant
3. View Results
4. See:

My only inkling is I may have created these particular symbols by duplicating another and then renaming it. Perhaps it's picking up some history there and erroneously flagging it?

cc @jedrichards @carmenlanza

These were some of the first rules written so they could be improved for readability/quality. Also they are at the top of the rules folder so might be first ones people click on when investigating the rules syntax in the repo!

This is so a rule can be toggled on and off with allObjects, and any state in objects won't be lost.

Activate the following rules

• No disabled borders
• No disabled fills
• No disabled shadows
• No disabled inner shadows
• No hidden layers
• No unused shared styles
• No unused symbols
• No dirty layer styles
• No dirty text styles
• No empty groups
• No redundant groups
• No loose layers outside artboards
• Maximum layers in a group
  • Set max to 10
• Maximum ungrouped layers in an artboard
  • Set max to 5
• Subpixel positioning
  • Allow .5 increments only

Also include

• Tests
• Rule documentation in package readme

There has been a lot of user research and feedback from designers saying that detecting the presence of "detached symbols" will be a super useful rule to have.

So this means detecting when a user has clicked "Detach from symbol" on a symbol instance, and converted it to a plain group, breaking the link with the symbol master. This can be a major problem in large organisations that require strict adherence to design systems using libraries and symbols.

This issue is to discuss how to implement a rule that achieves this. This is tricky because, as far as I know, once a symbol has been detached and converted into a group there is no record in the file JSON that the group was ever previously a symbol instance.

One idea has been to update Sketch to record a note in the group's userInfo property that it was previously a symbol instance ...

Or is there another way?

For each Assistant there is a 'Click here to add to your Sketch document'.

This was working up until last night. But now displays an error saying check the URL.

If you actually check the URL it returns:

'HTTP 500: Cannot read property 'latest' of undefined"

These assistants provided by Sketch seem nice and I really love that there are recommendations on artboard naming and such, but I'd really love to see a bit of reasoning or examples behind the decisions. Is there a "good conventions" guide somewhere that I'm missing as just seeing this:

Artboard names should start with numbers followed by a space

and then seeing this when I click the question mark:

Artboard names should start with numbers followed by a space, e.g. 1.1 Splash Screen.

does not tell me why I should be doing this and in what cases.

After seeing this rule, I'm left wondering is there some good convention I have missed? Is there a specific use case that does not apply to us?

I'd hope there be a bit more documentation on the linked .md, or even a link to a blogpost explaining the reasonings behind these default settings.

We should follow our own best practices and add primary categories to our three Assistants:

https://developer.sketch.com/assistants/publishing#best-practices

This will enhance the presentation of the Assistants on sketch.com too.

After adding the category keywords add a changeset and publish the Assistants so the new keywords are live on npm.

Suggested primary keywords:

• Naming Conventions guidelines
• Reuse Suggestions organization
• Tidy organization

"No blend mode in exported layers" should not yell at me if the exported file is going to be a bitmap Source

QA

Perhaps no QA required, just realised this rule is not present in a "live" Assistant. Enough that the tests are passing for now.

Filtering ignored objects at the file object iteration loops catches the vast majority of cases, but seems wise to have a fail safe of some kind.

That fail safe could be to filter out violations relating to ignored objects after the fact, but as we discovered before that could lead to inconsistent results and messages.

Raising a RuleError will increase visibility of the problem, and Assistant developers can fix.

Nothing happens when "Show in Document" is clicked.

An example of this are the *-prefer-shared style rules. These will generate an individual violation for every layer with matching layer style. Leading to feedback like:

Proposed solution:

Update violations so they reference an array of nodes, no longer just a single node.

This will require changes to the types, utils and maybe assistants too (unless the change can be made in a non-breaking fashion).

Once the changes are made we should update Sketch to support the new data structures.

It seems that for some users library names are likely not a robust enough approach, and there should be the option to alternatively use library ids.

Investigate approaches for not mutating the Sketch document objects with $pointer values. Could these be stored in a Map for example? E.g. mapping pointers to object references, and vice versa. We could do away with the whole concept of a Node and just work with clean native Sketch object types from the file format.

Rename the file property in the rule context so accessing the current SketchFile object from a rule doesn't look awkward, like

context.file.file

Add the following rules

• Page names
  • Should start with an emoji, e.g. "♻Symbols"
• Artboard names
  • Should start with a numbering system e.g. "1.1 Splash screen"
• Group names
  • Not default value "Group"
• Symbol names
  • Must use the "/" based symbol organisation mechanic

Also include

• Tests
• Rule documentation in package readme

Since we decided against using a web service helper to facilitate Assistant installation from web pages, this means some additional requirements around the "Add this Assistant to your Sketch document" button on Assistant details pages on sketch.com.

Ultimately, when a user clicks to add an Assistant they need to open a link in the format:

sketch://add-assistant?url=<assistant-tarball-url>

Where assistant-tarball-url is the full url to the current latest version of that Assistant, as a downloadable tarball. The only way to get this tarball url is to communicate with npm - and without a web service helper, we have to do it clientside (either when the user clicks the install button, or on page load or similar):

Pseudo code something like the below (missing click handler, error handling etc):

const pkg = '@sketch-hq/sketch-tidy-assistant' // Package name for current detail page
const res = await fetch(`https://registry.npmjs.org/${pkg}`, {
  headers: { Accept: 'application/vnd.npm.install-v1+json' }, // This header returns a minimal response
})
const data = await res.json()
const latestVersion = data['dist-tags'].latest
const tarball = data.versions[latestVersion].dist.tarball
const sketchLink = `sketch://add-assistant?url=${tarball}` // Open/redirect to this link

QA master branch and ensure that the three Assistants built on it work with Sketch Private.

All being well we can merge the release PR.

I tried to make a custom rule which takes an object as a config, but I'm getting a typescript error because it looks like the RuleOption type is defined as:

/**
 * The valid set of types available for individual rule options.
 */
export declare type RuleOption = string | number | boolean | string[] | {
    [key: string]: Maybe<string | number | boolean | string[]>;
}[];

Which means I would have to pass an array of objects, not just a single object, which seems odd. Is this actually intended, or just a typing bug?

Add the following rules

• Prefer shared layer styles
  • Tolerate 2 identical layer styles
• Prefer shared text styles
  • Tolerate 2 identical text styles
• Prefer symbols over groups
  • Tolerate 2 identical groups

Also include

• Tests
• Rule documentation in package readme

Assistants will benefit from web services that performs the following tasks:

1. Facilitates one-click installs (required for launch)
2. Sits in-between Sketch and the npm registry API, and provides Assistant package metadata (only required once we add support for Npm Assistants to Sketch, not required while we only support Url Assistants)

Read on more for more details.

Glossary

• Url Assistant An Assistant added to a Sketch document via a name and a fully qualified url to a hosted tarball. This sort of Assistant is technically unversioned, it just exists at a url.
• Npm Assistant An Assistant added to Sketch via a npm package name and semver. This sort of Assistant is versioned, and Sketch will be able to let a user know when the Assistant in their document can be updated etc.

One click installs

Using a web service here, instead of users crafting the sketch:// protocol deep-links themselves gains us the following advantages:

• One click install links can be added to websites that strip non-standard protocols from links (i.e. GitHub)
• We can find out the latest version of the package automatically, rather than a user having to keep their links up to date
• If we need to change something about the implementation in Sketch, or adjust the protocol we have a central place to do that, without invalidating links in use by 3rd parties
  • Example of this will be when we transition from only supporting Url Assistants to supporting Npm Assistants we can just make that change in this web service and change the behaviour centrally

Already have a proof of concept that is being used in the wild at the moment, so should just be a case of reworking this.

Url Assistants behaviour

This is the behaviour of the current proof of concept.

1. User clicks a link to the service, indicating the npm package name of the Assistant they want to add to their Sketch document
2. Various options can be supplied (e.g. tag or latest version etc, Sketch variant for the deep link etc.)
3. The service queries the npm API to resolve a fully qualified tgz url for that Assistant package
4. The service redirects to the sketch:// deep link protocol, and asks the Sketch to add it as a Url Assistant

Npm Assistant behaviour

This won't be required until we support Npm Assistants.

Path and options would change minimally between the two behaviours, so we could just switch it over when Npm Assistants are released.

1. User clicks a link to the service, indicating the npm package name of the Assistant they want to add to their Sketch document
2. Various options can be supplied (e.g. tag or latest version etc, Sketch variant for the deep link etc.)
3. The service queries the npm API to determine which semver should be used
4. The service redirects to the sketch:// deep link protocol, and asks Sketch to add it as a Npm Assistant

Sketch Npm Proxy

This won't be required until we support Npm Assistants.

This could begin life as a single endpoint, where Sketch could ask for metadata about Npm packages, and get back information.

Using a web service here will insulate Sketch from any changes or rough edges in the npm API.

• List available versions for each Assistant
• Expose human readable metadata for each Assistant for display in Sketch UI
• List full tarball urls for each version
• List any tags (there'll always be at least a latest tag, for example)

So if Sketch posted the following JSON to the endpoint (i.e. one or more Assistant names)

[
  "@sketch-hq/sketch-tidy-assistant"
]

It would get back something like

{
  "@sketch-hq/sketch-tidy-assistant": {
    "title": "Tidy Assistant",
    "description": "Blah",
    "icon": "https://www.domain.com/icon.png",
    "tags": {
      "latest": "5.0.1"
    },
    "versions": {
      "5.0.1": "https://registry.npmjs.org/@sketch-hq/sketch-tidy-assistant/-/sketch-tidy-assistant-5.0.1.tgz",
      "5.0.0": "https://registry.npmjs.org/@sketch-hq/sketch-tidy-assistant/-/sketch-tidy-assistant-5.0.0.tgz",
      "4.1.0": "https://registry.npmjs.org/@sketch-hq/sketch-tidy-assistant/-/sketch-tidy-assistant-4.1.0.tgz",
      ...
    }
  },
  ...
}

Technology choices

• Are we happy with Vercel?
  • It natively supports TypeScript, which is nice
• Are there other options? Netlify functions?
  • Netlify functions seem a little more basic than Vercel (no edge caching, locked to US AWS region by default, no TypeScript support)
• Something more homegrown, and/or owned by our backend team?
• Scalability?

Domain

Perhaps something like assistants.sketch.com?

A Node CLI for running Assistants outside of Sketch.

This should simplify working with option values in rules a bit, and technically be more typesafe ... users can add any value to a config, so in truth the value is unknown.

Currently they are pointing to a non-existent location in the GitHub repo.

QA

1. Clone this repository
2. Package the 3 assistants into local tarballs
3. Install these to a Sketch document using the Install from Disk option
4. Ensure the Assistant "Learn more" buttons do not 404 and lead to the correct pages

Similar to layer-styles-prefer-shared a new rule should suggest to use colour variables.

It should expose one option maxIdentical to allow people to define how often a colour value can be set within a document without nudging to use colour variables.

Sketch default rules artboards-layout and artboard-grids mark artboards as non-compliant when I turn off the visibility(!) of the layout or grid. I think it only should check the numbers and if the layout settings are on (checkbox) and not the visibility state (designers keep turning layout/grid on and off all the time and assistant notification changing their state based on this is misleading).

Here is my assistant:
https://github.com/carbon-design-system/carbon-sketch-assistant

Would be great to have an opportunity to override ruleDescription. It will allow to teams set their own tone of voice inside assistants

I found that when I provide a sketch object to utils.report, the text is chopped off after a very few characters, no matter how
wide I make the window:

You can see in the above, that the full text is kept in the hover/tooltip.

If I don't pass an object as the second parameter to utils.report, I get the full text: