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 Angular sample #847

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sebtoombs
Copy link

What

This PR adds a simple Angular app to the examples directory. The example closely copies the webpack-react example. The implementation should probably be considered as a reasonably robust proof of concept - it has not yet been used in production and has only been "tested" by one author (me).

Why

For better or worse, some of us need to use Angular. Most existing Angular component libraries are poor quality. We are building an Angular component library for internal use, and I wanted to explore if it was possible to use a best-in-class styling solution such as vanilla-extract.

How

The readme & webpack config cover most of the how, but tl;dr - follow the vanilla-extract install instructions for webpack, but make a slight modification to the Angular webpack config to avoid letting the angular webpack loader load .css.ts files. It's possible this could be done by modifying the Angular webpack loader to add an "emitter" for these files (if one knew how...).

@sebtoombs sebtoombs requested a review from a team September 26, 2022 06:42
@changeset-bot
Copy link

changeset-bot bot commented Sep 26, 2022

⚠️ No Changeset found

Latest commit: e06069d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@mattcompiles mattcompiles left a comment

Choose a reason for hiding this comment

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

Hey Bud, Thanks for the PR. This repo uses is pnpm for package management. Could we drop the package-lock.json file?

@sebtoombs
Copy link
Author

Hey Bud, Thanks for the PR. This repo uses is pnpm for package management. Could we drop the package-lock.json file?

Thanks Matt 🙏 sorry I wasn't sure whether I should update the root lock, as Angular often doesn't play well with others in this scenario (in my experience), and then I forgot to do anything about it.

I've updated to remove the package-lock.json and re run pnpm install to update the lockfile. I'm not sure what the best way to ensure this creates no issues with other packages is in this project.

@michaeltaranto michaeltaranto added the documentation Improvements or additions to documentation label Feb 27, 2023
Copy link
Contributor

@askoufis askoufis left a comment

Choose a reason for hiding this comment

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

Just some small comments, primarily to do with trimming down the boilerplate/generated code to keep the example as small as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this file when we already have global.css.ts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can the comments in this file be removed?

@@ -0,0 +1,22 @@
import { Attribute, Component, HostBinding } from '@angular/core';
import { style } from '@vanilla-extract/css';
import { sprinkles } from '../sprinkles.css';
Copy link
Contributor

Choose a reason for hiding this comment

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

sprinkles isn't used anywhere in this file. I think it would be a good idea to use sprinkles at runtime somewhere though, as the webpack example uses it both at buildtime and runtime, but this example currently only uses it within app.css.ts.

Comment on lines +1 to +3
.angular {
font-style: italic;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be moved to global.css.ts.

Copy link
Contributor

Choose a reason for hiding this comment

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

This could probably be trimmed down to just /dist and /.angular/cache. The rest of the stuff isn't really relevant and/or is handled by the repo-root gitignore.

Comment on lines +1 to +5
node_modules
dist
.angular
.vscode
README.md
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, probably only .angular needs to be here`.

@askoufis
Copy link
Contributor

@sebtoombs Are you still interested in getting this PR through?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants