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 types, allow bullets for description #35

Closed
wants to merge 1 commit into from
Closed

feat: add types, allow bullets for description #35

wants to merge 1 commit into from

Conversation

OmitNomis
Copy link
Contributor

@OmitNomis OmitNomis commented Dec 31, 2023

Add and fix some types,

Closes #23

Allows use of string or an array of string for desc, using an array creates bullet points. the bullets can be customized.

This is what the config would look like:

image

Preview:

When no customBullet is provided

image

When customBullet is provided

image

Copy link

vercel bot commented Dec 31, 2023

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

Name Status Preview Comments Updated (UTC)
cv ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 31, 2023 7:41am

Copy link

Choose a reason for hiding this comment

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

adding types is going to cause a lot of problems for people who just want to fork the project and use it. some users may not have a project, websiteurl or don't want to put out location links etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see how adding types would be a problem.
but you're right, people might not have everything. I'll see and change the types to optional for whatever is being conditionally rendered.

Comment on lines +72 to +75
{React.createElement(
social.icon as React.ComponentType<{ className: string }>,
{ className: "h-4 w-4" },
)}
Copy link
Owner

Choose a reason for hiding this comment

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

what's the reason for this change? what was the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only if we're adding types.
I'll remove this if types aren't necessary.

@@ -0,0 +1,64 @@
import { StaticImageData } from "next/image";
export type ResumeData = {
Copy link
Owner

Choose a reason for hiding this comment

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

having that adds pretty much no value but adds a lot of overhead In maintaining the type. using as const ensures we have a type to check against and I'd rather not include this change because of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I added types is because exporting as const won't allow as much customization.

For Example:
If I have any desc with a field customBullet and i use it in the page.tsx it works fine, because the field exists.

if I remove the field from the config, we get this error Property 'customBullet' does not exist on type 'never'.

The app still works fine, but Vercel doesn't allow this.

and for possibility of making the desc both array or string, exporting as const won't allow it unless there's atleast one array and one string in the whole config, or it'll have the type never, and the deployment fails.

@BartoszJarocki
Copy link
Owner

I suggest keeping the changes in your own fork as I see maintaining the explicit types as not needed overhead.

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.

Customisation for Description
3 participants