-
Notifications
You must be signed in to change notification settings - Fork 0
Split CVPreview into modular components with individual edit buttons #127
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for smart-cv-migracode ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
inkpot-monkey
left a comment
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.
Hi @SyedArslanHaider this is a great first step but we still have a lot of duplication that we can remove.
The current approach:
- Separates display components (CVHeader, CVEducation, etc.) from editing functionality
- Uses a large EditForm component that handles all editing scenarios
- Requires coordinating edit states between multiple components
Instead we should give each CV section component the ability to edit itself:
// CVHeader.jsx - with built-in editing
const CVHeader = ({ fullName, contact, onSave }) => {
const [isEditing, setIsEditing] = useState(false);
// When "Edit" is clicked, the component transforms into its own edit form
if (isEditing) {
return (
<div className={styles.section}>
{/* Edit form fields for this section only */}
<input value={fullName} onChange={...} />
{/* Save/Cancel buttons */}
<button onClick={() => { onSave(updatedData); setIsEditing(false); }}>
Save
</button>
</div>
);
}
// Normal display mode with an edit button
return (
<div className={styles.section}>
<h1>{fullName}</h1>
<button onClick={() => setIsEditing(true)}>Edit</button>
{/* Display contact info */}
</div>
);
};This gives the following advantages
- Simpler for Users - Edit exactly where you see the content
- Focused Editing - Only edit one section at a time
- Cleaner Code - Each component handles its own concerns
- No Redundancy - No duplicate EditForm component needed
- Easier Maintenance - Changes to a section stay within that file
To do this we need to:
- Move edit form fields from EditForm into their respective component files
- Add local editing state to each component (isEditing)
- Add save/cancel handlers that update the parent component's data
It would also be good to move the css for each component into separate module.css with only shared css remaining in CVPreview.module.css
Does this sound good? Do you have any questions or ideas about it?
…erate css for all
|
Hi @inkpot-monkey I try to fix the code can you please review it again ? |
inkpot-monkey
left a comment
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.
Hi @SyedArslanHaider great work here.
I have two comments, the first one needs to be changed in this PR and is that we have duplicated CSS declarations in the different module.css files such as
.edit-button {
background-color: var(--color-azulblue);
color: white;
border: none;
padding: 0.5rem 1rem;
border-radius: 0.25rem;
cursor: pointer;
font-size: 0.875rem;
display: flex;
align-items: center;
gap: 0.5rem;
transition: background-color 0.3s ease;
}
.edit-button:hover {
background-color: var(--color-royalblue);
}any duplicate css declarations should be put in CVPreview.module.css and imported from there to reduce duplication and only CSS declarations unique for the different sections should go in the different files.
- We dont have any validation for this form (we should just be able to reuse the yup validation for the input form but this can happen in a different PR)
Changes Made: