-
Notifications
You must be signed in to change notification settings - Fork 986
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
feat: add types, allow bullets for description #35
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
{React.createElement( | ||
social.icon as React.ComponentType<{ className: string }>, | ||
{ className: "h-4 w-4" }, | ||
)} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 = { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I suggest keeping the changes in your own fork as I see maintaining the explicit types as not needed overhead. |
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:
Preview:
When no
customBullet
is providedWhen
customBullet
is provided