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

feat: add layer dependency filtering options #207

Merged
merged 6 commits into from
Jan 23, 2020

Conversation

rickharris
Copy link
Contributor

Depends on https://github.com/goabstract/projects/pull/3894, posting the same explanation here for visibility

This adds two new filtering options for layers:

  • excludeLibraryDependencies - this will make it so that symbols from external libraries are not included in the list of layers. These are the items that we show as "dependencies" in the file list and aren't shown by default unless you click on them.
  • onlyLibraryDependencies - the opposite of excludeLibraryDependencies. You can choose this option if you need to load the library dependencies separately.

These options should help our pagination of layers be a little more intuitive so that we don't load pages of data that aren't even shown in the screen you're looking at.

@rickharris
Copy link
Contributor Author

I can't say I understand this test failure. @bitpshr have you seen that MultiError before?

Copy link
Contributor

@bitpshr bitpshr left a comment

Choose a reason for hiding this comment

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

Looks good so far. The SDK throws a MultiError instance that contains references to the underlying API and CLI error, if either exist. If you log that error you should see what's actually going wrong. After a quick glance, it looks like the tests are failing because the test mocks need to be updated in Layers.test.js. Since you added new HTTP parameters, those mocked URLs should be updated to include them. Also, can you please update the TypeScript definition file and the documentation in abstract-api.md to line up with the new filter options?

@@ -851,7 +851,7 @@ A layer is a container for designs. In Sketch a layer usually represents an artb

![CLI][cli-icon] ![API][api-icon]

`layers.list(FileDescriptor | PageDescriptor, ListOptions): Promise<Layer[]>`
`layers.list(FileDescriptor | PageDescriptor, LayersListOptions): Promise<Layer[]>`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no other documentation for what this type is, should I add it somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Our documentation is a mess, this will hopefully be fixed soon. For now, feel free to define this where ListOptions is defined towards the end of this document.

offset: 0
offset: 0,
excludeLibraryDependencies: false,
onlyLibraryDependencies: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added these here just to say that they exist but they're probably very out of place.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine for now. When we generate API documentation (planned), this will be shown in the right spot.

@tommoor
Copy link
Contributor

tommoor commented Jan 16, 2020

Did you consider using a single filter parameter for this? It feels like it would be more suited considering it doesn't make sense to set both flags at once… eg:

  • filter=undefined = returns all results as today
  • filter=all = returns all results as today
  • filter=dependencies = returns only dependencies
  • filter=normal = returns regular non-dependencies ("normal" is up for grabs here)

@rickharris
Copy link
Contributor Author

The same was suggested in the backend PR https://github.com/goabstract/projects/pull/3894#discussion_r367148849

I'll update

@tommoor
Copy link
Contributor

tommoor commented Jan 16, 2020

Oh, lol – that was later down my list of PR's to get to 😄

@rickharris
Copy link
Contributor Author

updated the option to fromLibrary: "include" | "exclude" | "only" in 2ecbc1c

src/endpoints/Layers.js Outdated Show resolved Hide resolved
Copy link
Contributor

@bitpshr bitpshr left a comment

Choose a reason for hiding this comment

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

Looks good to me pending Tom's typo fix.

Co-Authored-By: Tom Moor <tom@abstract.com>
@rickharris rickharris merged commit 88705e8 into master Jan 23, 2020
@rickharris rickharris deleted the layer-dependency-options branch January 23, 2020 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants