Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

feat(middleware): Add 'symbolMiddleware' and 'symbolInstanceMiddleware' #52

Merged

Conversation

mnikkane
Copy link
Contributor

@mnikkane mnikkane commented Jun 21, 2018

This allows for example setting static ID for the symbol master, which seems to fix at least partially #18. This also allows modifying symbol instances. Usage guide is provided in the readme.

Done similarly as symbol layers middleware in PR #39

@markdalgleish
Copy link
Member

Thank you so much for your PR! This looks really great.

I feel like we should also add symbolMiddleware at the same time to make this a complete feature. Are you open to doing this work? If not, I'm happy to find some time to pick up this branch.

@mnikkane
Copy link
Contributor Author

Hi, I could check that, should be good addition.

As the codebase is not that familiar to me, just double checking that do you mean adding middleware for symbol instances around https://github.com/seek-oss/html-sketchapp-cli/blob/master/script/generateAlmostSketch.js#L35 and passing node and symbolMaster as a parameters? Maybe name this new middleware to symbolInstanceMiddleware (or is symbolMiddleware enough)?

@markdalgleish
Copy link
Member

I was thinking just regular symbols (data-sketch-symbol), but it makes sense for symbol instances too, so I'd say we need both symbolMiddleware and symbolInstanceMiddleware.

@mnikkane
Copy link
Contributor Author

Does't the symbolMasterMiddleware already handle the regular symbols (data-sketch-symbol) in line 97: https://github.com/seek-oss/html-sketchapp-cli/pull/52/files#diff-7c5a4658631b0df813b5f5188bed6f16R97

Or am I missing something here 🤔?

@markdalgleish
Copy link
Member

Ah sorry, you're right. Forgot that all non-instance symbols are technically symbol masters.

I think the API should mirror the terminology we use in the data attributes.

<div data-sketch-symbol="..."> goes through symbolMiddleware.
<div data-sketch-symbol-instance="..."> goes through symbolInstanceMiddleware.

@mnikkane
Copy link
Contributor Author

Renamed the symbolMasterMiddleware to symbolMiddleware and added also symbolInstanceMiddleware (as I do not have own use case for it, so please tell if improvements needed).

Also fixed tests to pass with html-sketchapp 3.3.0 (hasClippingMask and clippingMaskMode was added added to asketch output in 3.3.0)

@markdalgleish markdalgleish changed the title feat(symbolMasterMiddleware): Add symbolMasterMiddleware API feat(middleware): Add 'symbolMiddleware' and 'symbolInstanceMiddleware' Jul 2, 2018
@markdalgleish markdalgleish merged commit 5818868 into seek-oss:master Jul 2, 2018
@markdalgleish
Copy link
Member

Thanks so much for this PR, and for your patience! 👏🏻

I refactored the code a little, tweaked the documentation, and published as v0.6.0 🚀 🎉

@mnikkane mnikkane deleted the feature/symbol-master-middleware branch July 3, 2018 05:13
@mnikkane
Copy link
Contributor Author

mnikkane commented Jul 3, 2018

Nice refactorings, excellent.

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants