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

Add zacs:style, strip zacs declarations from output #14

Merged
merged 10 commits into from
Jul 13, 2020

Conversation

radex
Copy link
Collaborator

@radex radex commented Jul 8, 2020

  • Update dependencies
  • Add new way of adding styles to a component - zacs:style={{ width: 10}}. This feels a lot simpler, more natural than defining separate props for individual style attributes... I'm considering whether namespacing is even necessary - we could make it a style={} attribute, and it would feel just right. But I'm not sure, so I'm taking the conservative path
  • Improve documentation
  • Strips ZACS declarations (const Foo = zacs.foo(...)) from output code, by default, since it doesn't do anything anyway. I initially considered to only do this in production mode, since accidentally making a (non-transpired) component out of a ZACS declaration would call the ZACS runtime to show a helpful error. But on the other hand, Blah is not defined also tells you something is wrong, and there's nothing more frustrating than having your code work perfectly fine in development, but break builds in CI or whenever you get to make production builds.

Comment on lines 80 to +82
+ "platform": "web", // or "native"
+ "production": true // pass `false` to enable debug attributes
+ "production": true, // pass `false` to enable debug attributes
+ "keepDeclarations": false // pass `true` to keep zacs.xxx variable declarations in output
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all properties required? Maybe it would be a good idea to highlight all optional props and their default values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nope, not required - but so what?

Copy link
Contributor

Choose a reason for hiding this comment

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

To indicate what's important and what can be configured later. It might be helpful for newcomers fiddling with the lib

@michalpopek michalpopek assigned radex and unassigned michalpopek Jul 13, 2020
@radex radex assigned michalpopek and unassigned radex Jul 13, 2020
@michalpopek michalpopek assigned radex and unassigned michalpopek Jul 13, 2020
@michalpopek
Copy link
Contributor

@radex our discussion should not block merging - do you want me to merge it?

@radex
Copy link
Collaborator Author

radex commented Jul 13, 2020

@michalpopek yep!

@michalpopek michalpopek merged commit b18e143 into master Jul 13, 2020
@michalpopek michalpopek deleted the codegen-improvements branch July 13, 2020 12:51
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.

None yet

2 participants