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: Adding our CSS classes to Tailwind's intellisense #673

Merged
merged 21 commits into from
Jan 9, 2023
Merged

Feat: Adding our CSS classes to Tailwind's intellisense #673

merged 21 commits into from
Jan 9, 2023

Conversation

AdrianGonz97
Copy link
Member

@AdrianGonz97 AdrianGonz97 commented Dec 11, 2022

Before submitting the PR:

  • Does your PR reference an issue? If not, please chat to the team on Discord or GitHub before submission.
  • Did you update and run tests before submission using npm run test?
  • Does your branch follow our naming convention? If not, please amend the branch name using branch -m new-branch-name
  • Did you update documentation related to your new feature or changes?

What does your PR address?

Progress tracking and experimenting for #650.

Running

  • Be sure to npm install to add the new dependencies
  • Run npm run build:jss to transpile the all.css stylesheet
  • You'll need to reload the editor so that TW's extension can restart and detect the plugin

@vercel
Copy link

vercel bot commented Dec 11, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
skeleton-docs ✅ Ready (Inspect) Visit Preview Jan 9, 2023 at 6:31PM (UTC)

@endigo9740
Copy link
Contributor

endigo9740 commented Dec 11, 2022

@niktek Given how much I'm juggling this week I'm going to set you as a reviewer on this one. I'd still like to make the final call and review before merge, but you may be able to jump in sooner than myself. See the primary conversation in the ticket to get caught up on this.

@endigo9740
Copy link
Contributor

endigo9740 commented Dec 27, 2022

@AdrianGonz97 this PR has been sitting idle for a couple weeks now. I'm not sure if you have any interest in returning to research CSS-in-JS transpiling. If not we might want to opt to close this out for now and return later.

However, please see my proposal here. It's not a silver bullet, and only get us part of the way there, but may be something we should look into sooner than later:
#650 (comment)

@AdrianGonz97
Copy link
Member Author

@endigo9740 I'm willing to give another deep dive into this again. I think I have fully recovered since my last CSS-in-JS venture 😂. Your suggestion in #650 (comment) seems like it should work! I'll give it a shot here.

@endigo9740
Copy link
Contributor

endigo9740 commented Dec 28, 2022

@AdrianGonz97 I know you're still working on this and testing things. But a quick review - from my perspective it looks like you're building a whole tool inside the Skeleton project to handle the transpiling process. I see quite a few additional commands, dependencies, scripts, etc. Something to consider here is is perhaps this would be better to split off to it's own package that Skeleton can then implement?

It can be something you can own completely if you so choose, or we can fold it under the umbrella of Skeleton. I have no strong feelings either way. But consider for a moment that if we're struggling with this issue, how many others might as well. It might be great if this is something that could be opened up and provided to others in a reusable fashion.

Let me know your thoughts!

@AdrianGonz97
Copy link
Member Author

@endigo9740 Most of the scripts I've added were part of my experimental phase, so I'll be deleting them once I get more dialed in on what's actually necessary. Same goes for the added packages. I'm not too sure that this would work as a separate package, but I can look into that later.

@AdrianGonz97
Copy link
Member Author

@endigo9740 This PR is almost complete.

One problem that I've been dealing with is the additional bundle size that this plugin adds on top of our already considerably large bundle. It was adding about ~30kB to the bundle by itself, which was unacceptable.

I figured out nice way to negate that completely by only allowing the plugin to function during development. Now we can have that sweet intellisense without the penalty of increasing our bundle size!

One last thing to do is to add your suggestion #650 (comment). Might be a bit difficult to do because I'll have to convert to CSS-in-JS. We'll see how it goes!

@AdrianGonz97 AdrianGonz97 marked this pull request as ready for review January 7, 2023 01:34
@AdrianGonz97
Copy link
Member Author

@endigo9740
Should be good to go now. Had to make a small change to prevent duplicate rules showing up in the intellisense because of #766, but that should be good now. We now have intellisense for all classes!

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