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

Separate parsing and core css functionality #10

Merged
merged 22 commits into from
Aug 10, 2021

Conversation

WorldSEnder
Copy link
Collaborator

As mentioned in #8 this introduces two crates (macro part not implemented), one for the representation of css (stylist-core) and the other for parsing and the yield_style in (stylist itself).

packages/stylist-core/Cargo.toml Outdated Show resolved Hide resolved
packages/stylist-core/src/ast.rs Outdated Show resolved Hide resolved
packages/stylist-core/src/registry.rs Outdated Show resolved Hide resolved
packages/stylist-core/src/style.rs Outdated Show resolved Hide resolved
packages/stylist-core/src/style.rs Outdated Show resolved Hide resolved
@WorldSEnder
Copy link
Collaborator Author

@futursolo alright, can you have a look over it and tell me if the design is okay to you so far, if there is anything missing that you want to see, and what should be documented better?

Copy link
Owner

@futursolo futursolo left a comment

Choose a reason for hiding this comment

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

@WorldSEnder

Thank you for your PR. I have written some thoughts on it.

/// // Example Output: stylist-uSu9NZZu
/// println!("{}", style.get_class_name());
/// let scopes = Default::default();
/// let style = Style::create_from_sheet("my-component", scopes);
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need to expose AST based Stylesheet API to public and having examples referencing them in the docs?

I was thinking about #[doc(hidden)] AST and anything that directly interacting with AST.

I guess that purpose of this is to be used with proc-macro, I cannot imagine that anyone would want to construct the Style this way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The main purpose of these methods is to advertise that you can create a Style without having to unwrap a Result, if you don't want to go through the (fallible) parser. I will add a comment to the docs about that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also should mention that the macro will probably return a Sheet, not a style directly, so then you'd use these methods.

packages/stylist-core/src/style.rs Outdated Show resolved Hide resolved
packages/stylist-core/Cargo.toml Outdated Show resolved Hide resolved
packages/stylist/src/style_ext.rs Outdated Show resolved Hide resolved
packages/stylist-core/src/registry.rs Outdated Show resolved Hide resolved
@WorldSEnder
Copy link
Collaborator Author

WorldSEnder commented Aug 9, 2021

I want to add some bits of documentation to style-core tomorrow now that the ast is semi-public, then mark it as ready for review merge.

@WorldSEnder WorldSEnder marked this pull request as ready for review August 9, 2021 14:05
@futursolo futursolo merged commit faa7587 into futursolo:master Aug 10, 2021
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.

2 participants