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: Add jsrepo #64

Merged
merged 20 commits into from
Dec 6, 2024
Merged

feat: Add jsrepo #64

merged 20 commits into from
Dec 6, 2024

Conversation

ieedan
Copy link
Contributor

@ieedan ieedan commented Dec 4, 2024

I am opening this up as a draft so we can discuss it.

This currently works in this state and you can test it by running:

# init then add
pnpm dlx jsrepo init --project --repos github/ieedan/geist/tree/jsrepo

pnpm dlx jsrepo add

# or

# add with zero-config
pnpm dlx jsrepo add --repo github/ieedan/geist/tree/jsrepo

We have to include utils and icons in the build so that we have all local dependencies covered but I have opted to not list them when users add blocks.

The problem that I want to discuss is icons...

With the current way that we do icons everything is imported into a index.ts file and then rexported to be used everywhere else. This does work, but because of this you have to install every icon even if you are only going to use 1.

So with that explained there are a few options:

  1. Convert each icon to a .svelte file and import each icon individually.
  2. Package the icons separately and install them as a dependency (I kinda like this one personally but it is more of a pain)

I don't think this really makes any sense to merge unless we make a change so that not every icon is installed for every component.

Edit: After ieedan/jsrepo#244 the time it takes to actually install isn't that bad but I would still find it annoying that every icon is installed in my repo after installing Avatar.

Copy link
Contributor

github-actions bot commented Dec 4, 2024

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
geist ✅ Ready (View Log) Visit Preview 7ab2d65

@ieedan
Copy link
Contributor Author

ieedan commented Dec 5, 2024

@shyakadavis any thoughts on this?

If we decided what to do I can try and implement it today!

@shyakadavis
Copy link
Owner

Hey, @ieedan I think I like the idea of installing all icons at once (If this is terrible, I am open to the alternative.) Here is my only reason; you know how Vite tries to optimize everything on the first page visit? When all icons are bundled and optimized, you can navigate to other pages without Vite trying to re-load and re-optimize on each visit. (It's done all at once; does that make sense?)

@ieedan
Copy link
Contributor Author

ieedan commented Dec 5, 2024

Hey, @ieedan I think I like the idea of installing all icons at once (If this is terrible, I am open to the alternative.) Here is my only reason; you know how Vite tries to optimize everything on the first page visit? When all icons are bundled and optimized, you can navigate to other pages without Vite trying to re-load and re-optimize on each visit. (It's done all at once; does that make sense?)

Yeah I have seen this with lucide it's super annoying!

@ieedan
Copy link
Contributor Author

ieedan commented Dec 5, 2024

Another problem that comes up with this is because many components will depend on every icon jsrepo will ask if you want to overwrite every icon almost every time you add a single component and that's pretty annoying.

Either way give it a try and let me know what you think!

These pop up in my project when I install
@ieedan
Copy link
Contributor Author

ieedan commented Dec 5, 2024

I guess a way around the annoying prompts for every icons is to just hold y or provide the -y flag.

Playing around with this adding some of the components to the projects works really good outside of the icons thing.

Just need to add some instruction for the other config files I think!

@ieedan
Copy link
Contributor Author

ieedan commented Dec 5, 2024

Also I relocated theme-switcher so that you can actually install it

@ieedan
Copy link
Contributor Author

ieedan commented Dec 5, 2024

Full demo: https://github.com/ieedan/geist-jsrepo

@ieedan
Copy link
Contributor Author

ieedan commented Dec 6, 2024

image

@ieedan
Copy link
Contributor Author

ieedan commented Dec 6, 2024

Okay no. 2 should be fixed in 1.17.6

@ieedan
Copy link
Contributor Author

ieedan commented Dec 6, 2024

I do think before we merge this we have to work on the instructions though. I have pretty detailed instructions in the demo README so maybe look there and let me know what you think?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@shyakadavis
Copy link
Owner

Snap. Why isn't "Tip" rendering like so?
Screenshot 2024-12-06 at 15 25 04

@ieedan
Copy link
Contributor Author

ieedan commented Dec 6, 2024

It should be:

Tip

tip

@ieedan
Copy link
Contributor Author

ieedan commented Dec 6, 2024

> [!TIP]
> tip

@ieedan
Copy link
Contributor Author

ieedan commented Dec 6, 2024

Yeah the tip is being a little b*tch

@ieedan
Copy link
Contributor Author

ieedan commented Dec 6, 2024

I thought it might of been line wrapping so I had claude shorten the tip but it didn't seem to make a difference

@ieedan
Copy link
Contributor Author

ieedan commented Dec 6, 2024

@shyakadavis So I think it's a bug with github markdown rendering. The rendering is different on PR previews for whatever reason. I just pasted this into the demo and it worked perfectly.

@ieedan ieedan marked this pull request as ready for review December 6, 2024 13:43
@ieedan
Copy link
Contributor Author

ieedan commented Dec 6, 2024

I think this is ready though unless you have any other issues that should be addressed first!

@shyakadavis shyakadavis self-requested a review December 6, 2024 18:17
Copy link
Owner

@shyakadavis shyakadavis left a comment

Choose a reason for hiding this comment

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

Thank you very much, @ieedan You're awesome.

@shyakadavis shyakadavis merged commit 37418e3 into shyakadavis:main Dec 6, 2024
4 checks passed
@ieedan
Copy link
Contributor Author

ieedan commented Dec 6, 2024

Thank you very much, @ieedan You're awesome.

Likewise my friend

@ieedan ieedan deleted the jsrepo branch December 6, 2024 18:35
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