Johanna Eriksson Portfolio #393
Conversation
…ions on how to do this in the README file. Add a folder for Components.
…ponents for the buttons, add App.css, update App.jsx to contain relevant code. Add Hero.css and Hero.jsx.
… grid. Create css files for each component. Set the structure by adding the components to different folders.
…ter.jsx. Create CSS files for each component, and add the components in own folders in the section folder. Add basic code to the section components.
…onents Hero, TextSection, and CardSection.
…to App.jsx, not FooterSection.
…icit props for passing content. Add information to the README file
…shed components. Add image for footer.
…ponent for Icons. Update Footer and Hero components to import the Icon.jsx. Update README.
…ithin the src folder. Import json data to the ProjectCard. Add hard coded data for the ArticleCard.
…tion. Add Content to the project.json. Add more styling to CardSection.
…p to display the h3 heading
…sktop and Mobile. Start adding responsive styling to the components
…tyling for font h2 for mobile
…nts in the components
Update README.
There was a problem hiding this comment.
Great job with this project!
You have reach almost all requirements but there are some small things that differs in your version from the design file.
Take a look at how you handle the spacing of your projects, because they stretch out too much. And on a larger screen it would look like this:
…200px to prevent project and article cards som stretching too much.
WalkthroughThe changes in this pull request involve the addition of several new files and the deletion of the old Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
🧹 Outside diff range and nitpick comments (35)
my-portfolio/src/components/ui/typography/Typography.jsx (2)
5-9: Enhance component documentation.While the current documentation explains the basic purpose, it would be beneficial to include:
- Usage examples
- Available props
- CSS class descriptions
/* -This component renders the Global headings and texts variables from the index.css +/** + * Typography components for consistent text styling across the application. + * Renders text elements using global styles defined in Typography.css + * + * @example + * <H1 heading="Welcome" /> + * <Body text="Lorem ipsum..." /> + * + * Available components: + * - H1: Large headings + * - H2: Medium headings + * - H3: Small headings + * - Body: Regular paragraph text + */
13-16: Consider adding flexibility to typography components.The components could benefit from accepting additional props for customization while maintaining consistent styling.
-export const H1 = ({ heading }) => <h1 className="heading1">{heading}</h1>; +export const H1 = ({ heading, className, ...props }) => ( + <h1 className={`heading1 ${className || ''}`} {...props}> + {heading} + </h1> +); -export const Body = ({ text }) => <p className="body-text">{text}</p>; +export const Body = ({ text, className, ...props }) => ( + <p className={`body-text ${className || ''}`} {...props}> + {text} + </p> +); +H1.propTypes = { + heading: PropTypes.string.isRequired, + className: PropTypes.string +};my-portfolio/src/components/ui/buttons/Button.jsx (4)
18-22: Add error handling for invalid URLs.The
handleClickfunction should handle potential invalid URLs to prevent unhandled exceptions.const handleClick = () => { if (link) { - window.open(link, "_blank"); + try { + // Validate URL format + new URL(link); + window.open(link, "_blank"); + } catch (error) { + console.error('Invalid URL:', link); + } } };
24-40: Consider adding loading state and keyboard interaction.The button implementation looks good, but could be enhanced with loading state and explicit keyboard handling for better user experience.
<button className={`button ${variant} ${fullWidth ? "full-width" : ""} ${className || ""}`} onClick={onClick || handleClick} aria-label={ariaLabel || label} + disabled={loading} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + onClick || handleClick(); + } + }} > {icon && ( <img src={icon} alt="" className="icon" aria-hidden="true" /> )} - <span className="label">{label}</span> + <span className="label"> + {loading ? 'Loading...' : label} + </span> </button>
31-37: Consider adding error handling for icon loading.The image element should handle loading errors gracefully to maintain the button's appearance even if the icon fails to load.
<img src={icon} alt="" className="icon" aria-hidden="true" + onError={(e) => { + e.target.style.display = 'none'; + console.error('Failed to load button icon:', icon); + }} />
26-26: Consider using CSS modules for better CSS encapsulation.The current className concatenation could lead to naming conflicts. Consider using CSS modules to scope styles to this component.
-import "./Button.css"; +import styles from "./Button.module.css"; -className={`button ${variant} ${fullWidth ? "full-width" : ""} ${className || ""}`} +className={`${styles.button} ${styles[variant]} ${fullWidth ? styles.fullWidth : ""} ${className || ""}`}my-portfolio/src/components/sections/footer/Footer.jsx (3)
1-7: Convert block comments to JSDoc format for better documentation.The current comment style could be improved by using JSDoc format, which would provide better IDE support and documentation generation.
-/* ========== FOOTER SECTION ========== */ - -/* This component contains the Footer, and contain an image, heading, contact information, and icons with social links. - -The icons and their links are imported from SocialIcons component. - -*/ +/** + * Footer section component that displays contact information and social links + * @component + * @description Renders a footer section containing a profile image, heading, + * contact information (name, phone, email), and social media icons. + */
18-18: Enhance image accessibility with more descriptive alt text.The current alt text could be more descriptive to better serve users with screen readers.
- <img src={placeholderFooterImage} alt="Footer profile image of Johanna Eriksson" className="footer-profile-image" /> + <img src={placeholderFooterImage} alt="Johanna Eriksson - Frontend Developer" className="footer-profile-image" />
25-25: Consider adding aria-label to SocialIcons component.The social links section could benefit from improved accessibility.
- <SocialIcons text="Social links" className="social-links" /> + <SocialIcons text="Social links" className="social-links" aria-label="Social media links" />my-portfolio/src/components/sections/hero/Hero.jsx (2)
1-6: Enhance component documentation using JSDoc standards.Consider using JSDoc format for better documentation and IDE support.
-/* ========== HERO SECTION ========== */ - -/* This component contains the Hero header. It contains headings, body text, an image and icons with links. -- The styling for the headings and body text is imported from the Typography component. -- The icons and their links are imported from the SocialIcons component. -*/ +/** + * Hero section component that displays the portfolio header. + * @component + * @description Renders a hero section with headings, body text, profile image, + * and social media icons. Uses Typography components for consistent text styling + * and SocialIcons for social media links. + * @returns {JSX.Element} Hero section component + */
14-14: Maintain consistent export pattern.The component has both named and default exports, which is redundant. Choose one export pattern for consistency.
-export const Hero = () => { +const Hero = () => { // component implementation }; export default Hero;Also applies to: 30-30
my-portfolio/src/components/ui/typography/Typography.css (3)
1-8: Enhance documentation with implementation details.While the current comments provide basic context, they could be more helpful by including:
- The location of CSS variable definitions
- Expected values or ranges for these variables
- Any responsive design considerations
Consider adding documentation like this:
/* ========== TYPOGRAPHY STYLING ========== */ /* * Typography component that applies consistent text styling across the app. * * Dependencies: * - Global CSS variables defined in src/index.css * - Font sizes follow a modular scale * - Responsive breakpoints: * - Mobile: Default values * - Tablet: Scale up by 1.1 * - Desktop: Scale up by 1.2 */
9-17: Consider adding color and responsive typography for h1.The h1 styles are well-structured but could be enhanced:
- Add a color property for consistency with other headings
- Consider adding responsive typography using media queries
h1 { font-family: var(--font-family-h1); font-size: var(--font-size-h1); font-weight: var(--font-weight-h1); line-height: var(--line-height-h1); font-style: var(--font-style-h1); + color: var(--color-h1); margin: 0; padding: 0; } + +@media (min-width: 768px) { + h1 { + font-size: calc(var(--font-size-h1) * 1.1); + } +}
19-40: Optimize heading styles for consistency and maintainability.The h2 and h3 styles have slight inconsistencies in margin handling. Consider:
- Standardizing margin approach across all headings
- Using CSS custom properties for margins
- Applying DRY principles to reduce repetition
:root { + --heading-margin: 0; + --heading-padding: 0; } h2, h3 { + margin: var(--heading-margin); + padding: var(--heading-padding); } h2 { font-family: var(--font-family-h2); font-size: var(--font-size-h2); font-weight: var(--font-weight-h2); line-height: var(--line-height-h2); font-style: var(--font-style-h2); color: var(--color-h2); - margin: 0; - padding: 0; }my-portfolio/src/components/sections/text-section/TextSection.jsx (2)
13-35: Extract content to a separate configuration file.The hardcoded content should be moved to a separate configuration file for better maintainability. Also, there's some duplication of tech skills between the two content structures.
Consider:
- Moving content to
src/config/content.js- Unifying the data structure
- Removing duplicated tech skills
// src/config/content.js export const portfolioContent = { skills: { title: "Skills", columns: [ { title: "Code", items: ["HTML5", "CSS3", "JavaScript ES6", "React", "Styled Components", "GitHub"] }, // ... other columns ] }, tech: { title: "Tech", description: "HTML, CSS, Flexbox, ..." } };
37-64: Consider breaking down into smaller components.The component could be split into
SkillsSectionandTechSectioncomponents for better maintainability and reusability.Example structure:
const SkillsSection = ({ columns }) => ( <div className="text-section-skills"> <H2 heading="Skills" /> <div className="skills-columns"> {columns.map((column) => ( <SkillColumn key={column.title} {...column} /> ))} </div> </div> ); const TechSection = ({ title, description }) => ( <div className="text-section-tech"> <H2 heading={title} /> <Body text={description} /> </div> ); export const TextSection = ({ category }) => { if (category === "skills") { return <SkillsSection columns={skillsContent} />; } if (category === "tech") { return <TechSection {...techContent} />; } return null; };my-portfolio/src/components/sections/card-section/CardSection.css (3)
1-7: Enhance documentation structure and clarity.While the comments provide good context, consider restructuring them for better readability and completeness.
-/* ========== CARD SECTION STYLING========== */ - -/* This component contains stylings for the cards displayed under Featured Projects and My Journey. The cards contain heading, image, body-text, and button/-s. - -The buttons are imported from Button.jsx. Layout for specific buttons can be found further down in this file. - -*/ +/* ========== CARD SECTION STYLING ========== + * Component: Card section for Featured Projects and My Journey + * + * Structure: + * - Card Container: Grid layout with 4 columns + * - Cards: Project cards and Article cards + * + * Components: + * - Heading + * - Image + * - Body text + * - Buttons (imported from Button.jsx) + * + * Dependencies: + * - Button styles from Button.jsx + */
9-21: Add responsive design for better mobile experience.The current layout might not adapt well to smaller screens. Consider adding media queries for better responsiveness.
.card-section { display: flex; flex-direction: column; align-items: center; padding: 4rem 0; width: 100%; gap: 2rem; + /* Add responsive padding */ + @media (max-width: 768px) { + padding: 2rem 1rem; + } } .card-section h2 { margin-bottom: 2rem; text-align: center; + /* Adjust font size for mobile */ + @media (max-width: 768px) { + font-size: 1.5rem; + margin-bottom: 1rem; + } }
32-46: Optimize transform animation performance.The current transform animation is good, but could be optimized for better performance.
.project-card, .article-card { display: flex; flex-direction: column; background-color: var(--natural-color); padding: 1.5rem; border-radius: 10px; box-shadow: 0px 4px 8px rgba(0, 0, 0, 0.1); width: 250px; gap: 1rem; - transition: transform 0.3s ease; + transition: transform 0.3s ease-out; + will-change: transform; } .project-card:hover, .article-card:hover { transform: translateY(-10px); + /* Add box-shadow transition for depth effect */ + box-shadow: 0px 8px 16px rgba(0, 0, 0, 0.15); }my-portfolio/src/components/sections/footer/Footer.css (3)
1-7: Enhance documentation with CSS variable dependencies.Consider adding documentation about the CSS variables being used (e.g.,
--natural-color,--primary-color, etc.) and their expected values. This helps maintainability and makes it easier for other developers to understand the styling requirements./* ========== FOOTER STYLING ========== */ /* This component contains styling for the Footer. The social icons and their links are styled via the SocialIcons component, except for a margin only used in the Footer. +Required CSS Variables: + --natural-color: Background color for the footer + --primary-color: Border color for profile image + --secondary-color: Text color + --font-family-h2, --font-size-h2, --font-weight-h2, --line-height-h2: Heading typography + --font-family-body, --font-size-body: Body text typography */
10-17: Consider using relative units for padding.The fixed padding values (
4rem 8rem 8rem 8rem) might be too large on smaller screens or too small on larger screens. Consider using relative units or viewport-based units for more flexible scaling..footer-section { display: flex; - padding: 4rem 8rem 8rem 8rem; + padding: clamp(2rem, 5vw, 4rem) clamp(4rem, 8vw, 8rem) clamp(4rem, 8vw, 8rem) clamp(4rem, 8vw, 8rem); justify-content: space-evenly; background-color: var(--natural-color); text-align: left; gap: 4rem; }
70-72: Use relative units for social icons margin.Consider using a responsive margin that scales with viewport size.
.footer-section .social-icons { - margin-top: 4rem; + margin-top: clamp(2rem, 4vw, 4rem); }my-portfolio/src/styles/index.css (2)
1-5: Add height: auto to maintain image aspect ratio.While the current implementation makes images responsive, adding
height: autowould ensure that images maintain their aspect ratio when scaling.img { display: block; max-width: 100%; + height: auto; }
1-155: Consider implementing a CSS reset and font preloading.The design system is well-structured, but could benefit from:
- A proper CSS reset (like normalize.css) to ensure consistent rendering across browsers
- Font preloading to prevent layout shifts:
<link rel="preload" href="/fonts/Urbanist.woff2" as="font" type="font/woff2" crossorigin> <link rel="preload" href="/fonts/Poppins.woff2" as="font" type="font/woff2" crossorigin>Would you like me to provide a complete CSS reset implementation or help set up font preloading?
🧰 Tools
🪛 Biome
[error] 58-58: Expected a declaration, or an at rule but instead found ';'.
Expected a declaration, or an at rule here.
(parse)
my-portfolio/src/components/sections/hero/Hero.css (4)
1-7: Enhance component documentation for better maintainability.Consider using a more structured documentation format that includes:
- Component purpose
- Required dependencies with their versions
- Usage examples
- Props/customization options
-/* ========== HERO SECTION STYLES ========== */ - -/* This component contains the styling for the Hero header. - -- The styling for headings and body text is imported from the Typography component. -- The styling for the icons and their links is imported from SocialIcons component. */ +/** + * Hero Section Styles + * + * Styles for the portfolio's hero header section. + * + * Dependencies: + * - Typography component: Provides heading and body text styles + * - SocialIcons component: Provides icon and link styles + * + * Usage: + * Import this CSS file into your Hero component: + * import './Hero.css' + */
21-45: Clean up text box styles and improve consistency.There are a few improvements that could be made:
- Remove duplicate
text-alignproperty in.text-box .body-text- Consider using CSS custom properties for consistent spacing
.text-box .body-text { margin-top: 2rem; - text-align: left; font-style: normal; font-weight: 400; line-height: 2rem; }
49-64: Enhance image box maintainability and performance.Consider these improvements:
- Use CSS custom properties for colors to maintain consistency
- Add
object-fitproperty to ensure proper image scaling.image-box { display: flex; align-items: center; justify-content: center; width: 19.5rem; height: 19.5rem; border-radius: 50%; overflow: hidden; flex-shrink: 0; - border: 0.25rem solid #FD6F00; + border: 0.25rem solid var(--primary-color, #FD6F00); } .profile-image { width: 100%; height: auto; + object-fit: cover; }
123-132: Clean up tablet styles and improve accessibility.Consider these improvements:
- Remove redundant
font-style: normaldeclarations- Convert pixel units to rem for better accessibility
.text-box h1 { - font-size: 5rem; - font-style: normal; + font-size: 3.125rem; /* 50px to rem */ line-height: 6.25rem; } .text-box h3 { - font-style: normal; line-height: normal; text-align: left; }my-portfolio/src/components/sections/card-section/CardSection.jsx (2)
79-96: Enhance accessibility and user experience of ProjectCard.Several improvements can be made to the ProjectCard component:
- Add ARIA labels for better accessibility
- Move gradient values to CSS variables
- Add loading states for external links
const ProjectCard = ({ name, content, image, netlifyLink, githubLink }) => ( - <div className="project-card"> + <div className="project-card" role="article" aria-label={`Project: ${name}`}> <div className="project-image" style={{ - backgroundImage: `linear-gradient(0deg, rgba(253, 111, 0, 0.30), rgba(253, 111, 0, 0.30)), url(${image})`, + backgroundImage: `linear-gradient(0deg, var(--overlay-color), var(--overlay-color)), url(${image})`, }} + role="img" + aria-label={`Screenshot of ${name}`} ></div>
107-107: Standardize date formatting.The date format should be consistent and localized. Consider using a date formatting library or the Intl API.
-<p className="date">{date}</p> +<p className="date"> + {new Intl.DateTimeFormat('en-US', { + dateStyle: 'long' + }).format(new Date(date))} +</p>my-portfolio/README.md (5)
1-5: Enhance the project introduction section.Consider expanding the introduction to include:
- Project purpose and goals
- Key technologies and frameworks used
- Brief overview of portfolio sections/features
- Target audience
14-18: Fix grammatical issues in the workflow section.Consider these corrections:
- "design were missing" → "design was missing"
- Use consistent articles: "a h1" instead of "an h1"
- Add commas after introductory phrases
Also, consider structuring this section with bullet points or numbered steps for better readability.
🧰 Tools
🪛 LanguageTool
[grammar] ~15-~15: Do you mean “designs”?
Context: ...uttons. It also made me notice that the design were missing an h1, and a paragraph tag...(DT_NN_ARE_AME)
[misspelling] ~15-~15: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ... me notice that the design were missing an h1, and a paragraph tag styled to look ...(EN_A_VS_AN)
[misspelling] ~15-~15: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...and a paragraph tag styled to look like an h3. After completing the sketch, I foc...(EN_A_VS_AN)
19-40: Restructure the Problems section for better organization.Consider these improvements:
- Move deployment challenges to a separate "Deployment" section
- Use consistent header formatting
- Break down long sentences for better readability
- Add a "Solutions" subsection for each problem
This would make the documentation more valuable for future reference.
🧰 Tools
🪛 LanguageTool
[style] ~21-~21: To elevate your writing, try using a synonym here.
Context: ...oding before that was set, as I find it hard to reorganize components and code later...(HARD_TO)
[style] ~21-~21: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...d code later, especially when there are a lot of changes to make. Hopefully spending tim...(A_LOT_OF)
[formatting] ~21-~21: Consider adding a comma after ‘Hopefully’ for more clarity.
Context: ...hen there are a lot of changes to make. Hopefully spending time on the structure will mak...(CONJUNCTIVE_LINKING_ADVERB_COMMA_PREMIUM)
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...ricky to set right, and I mixed up some names like using FooterSection when the compo...(AI_HYDRA_LEO_MISSING_COMMA)
[misspelling] ~27-~27: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...ucture. The original design was missing an h1 but included two different font size...(EN_A_VS_AN)
[uncategorized] ~33-~33: Possible missing comma found.
Context: ...g the Global styling** In a couple of places the different heading tags had differen...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~36-~36: ‘made a plan’ might be wordy. Consider a shorter alternative.
Context: ...et I spent over 20 hours on the course, made a plan, and kept a good pace, and had a lot of...(EN_WORDINESS_PREMIUM_MADE_A_PLAN)
[uncategorized] ~39-~39: It appears that hyphens are missing in the adjective “up-to-date”.
Context: .... Even Git reported that everything was up to date when I tried to rewrite the imports. In...(UP_TO_DATE_HYPHEN)
41-51: Fix grammar and prioritize improvements.
- Fix grammar: "I would used" → "I would use"
- Consider categorizing improvements by priority:
- High: Component reusability (Bio, Card components)
- Medium: Styled components implementation
- Nice-to-have: Card effects
This will help in planning future iterations.
🧰 Tools
🪛 LanguageTool
[grammar] ~46-~46: The modal verb ‘would’ requires the verb’s base form.
Context: ...Hero and Footer component. 3. I would used a component for the cards in the CardSe...(MD_BASEFORM)
[style] ~48-~48: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ards in the CardSection component. 4. I would create a component for Bio contai...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[formatting] ~48-~48: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...ticed this while working with the Footer, because I more or less were copying and pasting...(COMMA_BEFORE_BECAUSE)
52-59: Enhance the resources section.Consider expanding this section to include:
- Brief descriptions of how each resource was used
- Additional categories (documentation, tutorials, tools)
- Version information for key dependencies
🧰 Tools
🪛 Markdownlint
52-52: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (12)
.DS_Storeis excluded by!**/.DS_Storemy-portfolio/package-lock.jsonis excluded by!**/package-lock.jsonmy-portfolio/public/vite.svgis excluded by!**/*.svgmy-portfolio/src/assets/icons/Btn - github.svgis excluded by!**/*.svgmy-portfolio/src/assets/icons/Btn - instagram.svgis excluded by!**/*.svgmy-portfolio/src/assets/icons/Btn - linkedin.svgis excluded by!**/*.svgmy-portfolio/src/assets/icons/Btn - stackoverflow.svgis excluded by!**/*.svgmy-portfolio/src/assets/icons/Btn - twitter.svgis excluded by!**/*.svgmy-portfolio/src/assets/images/placeholder-footer-image.pngis excluded by!**/*.pngmy-portfolio/src/assets/images/placeholder-image.pngis excluded by!**/*.pngmy-portfolio/src/assets/react.svgis excluded by!**/*.svgpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
README.md(0 hunks)my-portfolio/.gitignore(1 hunks)my-portfolio/README.md(1 hunks)my-portfolio/eslint.config.js(1 hunks)my-portfolio/index.html(1 hunks)my-portfolio/package.json(1 hunks)my-portfolio/src/App.jsx(1 hunks)my-portfolio/src/components/sections/card-section/CardSection.css(1 hunks)my-portfolio/src/components/sections/card-section/CardSection.jsx(1 hunks)my-portfolio/src/components/sections/footer/Footer.css(1 hunks)my-portfolio/src/components/sections/footer/Footer.jsx(1 hunks)my-portfolio/src/components/sections/hero/Hero.css(1 hunks)my-portfolio/src/components/sections/hero/Hero.jsx(1 hunks)my-portfolio/src/components/sections/text-section/TextSection.css(1 hunks)my-portfolio/src/components/sections/text-section/TextSection.jsx(1 hunks)my-portfolio/src/components/ui/buttons/Button.css(1 hunks)my-portfolio/src/components/ui/buttons/Button.jsx(1 hunks)my-portfolio/src/components/ui/icons/SocialIcons.css(1 hunks)my-portfolio/src/components/ui/icons/SocialIcons.jsx(1 hunks)my-portfolio/src/components/ui/typography/Typography.css(1 hunks)my-portfolio/src/components/ui/typography/Typography.jsx(1 hunks)my-portfolio/src/main.jsx(1 hunks)my-portfolio/src/projects.json(1 hunks)my-portfolio/src/styles/index.css(1 hunks)my-portfolio/vite.config.js(1 hunks)projects.json(0 hunks)
💤 Files with no reviewable changes (2)
- README.md
- projects.json
✅ Files skipped from review due to trivial changes (11)
- my-portfolio/.gitignore
- my-portfolio/eslint.config.js
- my-portfolio/index.html
- my-portfolio/package.json
- my-portfolio/src/App.jsx
- my-portfolio/src/components/ui/buttons/Button.css
- my-portfolio/src/components/ui/icons/SocialIcons.css
- my-portfolio/src/components/ui/icons/SocialIcons.jsx
- my-portfolio/src/main.jsx
- my-portfolio/src/projects.json
- my-portfolio/vite.config.js
🧰 Additional context used
🪛 LanguageTool
my-portfolio/README.md
[grammar] ~15-~15: Do you mean “designs”?
Context: ...uttons. It also made me notice that the design were missing an h1, and a paragraph tag...
(DT_NN_ARE_AME)
[misspelling] ~15-~15: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ... me notice that the design were missing an h1, and a paragraph tag styled to look ...
(EN_A_VS_AN)
[misspelling] ~15-~15: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...and a paragraph tag styled to look like an h3. After completing the sketch, I foc...
(EN_A_VS_AN)
[style] ~21-~21: To elevate your writing, try using a synonym here.
Context: ...oding before that was set, as I find it hard to reorganize components and code later...
(HARD_TO)
[style] ~21-~21: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...d code later, especially when there are a lot of changes to make. Hopefully spending tim...
(A_LOT_OF)
[formatting] ~21-~21: Consider adding a comma after ‘Hopefully’ for more clarity.
Context: ...hen there are a lot of changes to make. Hopefully spending time on the structure will mak...
(CONJUNCTIVE_LINKING_ADVERB_COMMA_PREMIUM)
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...ricky to set right, and I mixed up some names like using FooterSection when the compo...
(AI_HYDRA_LEO_MISSING_COMMA)
[misspelling] ~27-~27: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...ucture. The original design was missing an h1 but included two different font size...
(EN_A_VS_AN)
[uncategorized] ~33-~33: Possible missing comma found.
Context: ...g the Global styling** In a couple of places the different heading tags had differen...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~36-~36: ‘made a plan’ might be wordy. Consider a shorter alternative.
Context: ...et I spent over 20 hours on the course, made a plan, and kept a good pace, and had a lot of...
(EN_WORDINESS_PREMIUM_MADE_A_PLAN)
[uncategorized] ~39-~39: It appears that hyphens are missing in the adjective “up-to-date”.
Context: .... Even Git reported that everything was up to date when I tried to rewrite the imports. In...
(UP_TO_DATE_HYPHEN)
[grammar] ~46-~46: The modal verb ‘would’ requires the verb’s base form.
Context: ...Hero and Footer component. 3. I would used a component for the cards in the CardSe...
(MD_BASEFORM)
[style] ~48-~48: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ards in the CardSection component. 4. I would create a component for Bio contai...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[formatting] ~48-~48: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...ticed this while working with the Footer, because I more or less were copying and pasting...
(COMMA_BEFORE_BECAUSE)
🪛 Markdownlint
my-portfolio/README.md
52-52: Punctuation: ':'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
🪛 Biome
my-portfolio/src/components/sections/text-section/TextSection.css
[error] 82-82: Unexpected shorthand property padding after padding-bottom
(lint/suspicious/noShorthandPropertyOverrides)
[error] 83-83: Unexpected shorthand property margin after margin-bottom
(lint/suspicious/noShorthandPropertyOverrides)
[error] 131-131: Unexpected shorthand property padding after padding-bottom
(lint/suspicious/noShorthandPropertyOverrides)
[error] 132-132: Unexpected shorthand property margin after margin-bottom
(lint/suspicious/noShorthandPropertyOverrides)
my-portfolio/src/styles/index.css
[error] 58-58: Expected a declaration, or an at rule but instead found ';'.
Expected a declaration, or an at rule here.
(parse)
🔇 Additional comments (5)
my-portfolio/src/components/ui/typography/Typography.jsx (1)
11-11: Verify Typography.css implementation.
Ensure that the imported CSS file defines all the required classes (heading1, heading2, heading3, body-text).
✅ Verification successful
Typography.css implementation is properly structured
The CSS file correctly implements all necessary styles:
- Heading styles are defined using h1, h2, and h3 selectors
- Body text style is defined with the .body-text class
- All styles use CSS variables for consistent theming
- Additional paragraph styling is included
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Typography.css exists and contains required classes
rg -l "\.heading[123]|\.body-text" "my-portfolio/src/components/ui/typography/Typography.css"
Length of output: 150
Script:
#!/bin/bash
# Let's check the actual content of Typography.css to verify the class definitions
cat my-portfolio/src/components/ui/typography/Typography.css
Length of output: 1505
my-portfolio/src/components/ui/buttons/Button.jsx (1)
8-41: Overall implementation looks good with room for enhancement.
The Button component is well-structured and includes good accessibility features. The implementation is clean and follows React best practices. The component is flexible with its props and handles different variants appropriately.
Let's verify the button's usage across the codebase to ensure consistent implementation:
✅ Verification successful
Let me gather more information about the Button component's implementation and usage patterns.
Let me try one more search to check for button-related styles and any other button usages:
Button component implementation and usage are consistent across the codebase
The verification shows that:
- The Button component is properly implemented with consistent prop usage
- It's correctly used in both ProjectCard and ArticleCard components
- The styling is well-structured with proper CSS classes for variants (outlined, filled) and width options
- The component's accessibility features (aria-labels) are maintained throughout
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Button component usage patterns
# Expected: Consistent usage of props across the codebase
# Check for Button imports and usage
rg -A 3 "import.*Button.*from.*['\"].*buttons/Button"
# Check for potential direct style overrides
rg -A 3 "className=\".*button.*\""
Length of output: 965
Script:
#!/bin/bash
# Check for all button-related CSS classes and their usage
rg -l "\.button\s*{"
# Check the actual implementation of buttons in CardSection
cat my-portfolio/src/components/sections/card-section/CardSection.jsx
# Look for any other potential button implementations
ast-grep --pattern 'Button = ({$$$}) => {$$$}'
Length of output: 3973
Script:
#!/bin/bash
# Check for button styles with fixed regex
rg "\.button" -A 3
# Check for any other potential Button imports or implementations
rg "Button" -l
# Check CSS file mentioned in CardSection
cat my-portfolio/src/components/sections/card-section/CardSection.css
Length of output: 4370
my-portfolio/src/components/sections/hero/Hero.jsx (1)
9-10: Verify Typography CSS import necessity.
The Typography CSS import might be redundant if it's already imported within the Typography component itself.
#!/bin/bash
# Check if Typography.css is imported in Typography component
cat src/components/ui/typography/Typography.jsx 2>/dev/null || echo "Typography component file not found"my-portfolio/src/styles/index.css (1)
36-45: LGTM!
The background styles are well-implemented with proper resets and overflow handling.
my-portfolio/src/components/sections/card-section/CardSection.jsx (1)
15-15: Add error handling for JSON data import.
The project data import could fail if the JSON file is malformed. Consider adding error handling and a fallback.
| @@ -0,0 +1,16 @@ | |||
| /* eslint-disable react/prop-types */ | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider disabling prop-types validation.
Instead of disabling prop-types validation globally, consider adding proper PropTypes or migrating to TypeScript for better type safety and documentation.
-/* eslint-disable react/prop-types */
+import PropTypes from 'prop-types';
+const headingPropTypes = {
+ heading: PropTypes.string.isRequired
+};
+const bodyPropTypes = {
+ text: PropTypes.string.isRequired
+};Committable suggestion skipped: line range outside the PR's diff.
| /* eslint-disable react/prop-types */ | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add PropTypes validation instead of disabling ESLint.
Rather than disabling prop-types validation entirely, it's better to explicitly define the expected props using PropTypes. This improves code maintainability and helps catch potential prop-related issues during development.
-/* eslint-disable react/prop-types */
+import PropTypes from 'prop-types';
+
+Button.propTypes = {
+ icon: PropTypes.string,
+ label: PropTypes.string.isRequired,
+ link: PropTypes.string,
+ className: PropTypes.string,
+ onClick: PropTypes.func,
+ ariaLabel: PropTypes.string,
+ variant: PropTypes.oneOf(['outlined', 'filled']),
+ fullWidth: PropTypes.bool
+};Committable suggestion skipped: line range outside the PR's diff.
| <Body text="+46(0)70-525 33 16" className="footer-phone" /> | ||
| <Body text="[email protected]" className="footer-email" /> |
There was a problem hiding this comment.
Publishing personal contact information (phone number and email) directly in the source code poses privacy and security risks. This information could be scraped and misused for spam or social engineering attempts.
Consider:
- Moving contact information to environment variables or a secure configuration
- Implementing contact forms instead of displaying direct contact details
- Using obfuscation techniques if direct display is necessary
- <Body text="+46(0)70-525 33 16" className="footer-phone" />
- <Body text="[email protected]" className="footer-email" />
+ <Body text={process.env.REACT_APP_CONTACT_PHONE} className="footer-phone" />
+ <Body text={process.env.REACT_APP_CONTACT_EMAIL} className="footer-email" />Committable suggestion skipped: line range outside the PR's diff.
| export const Footer = () => { | ||
| return ( | ||
| <div className="footer-section"> | ||
| <div className="footer-image-box"> | ||
| <img src={placeholderFooterImage} alt="Footer profile image of Johanna Eriksson" className="footer-profile-image" /> | ||
| </div> | ||
| <div className="footer-contact-info"> | ||
| <H2 heading="Let’s talk" /> | ||
| <Body text="Johanna Eriksson" className="footer-name" /> | ||
| <Body text="+46(0)70-525 33 16" className="footer-phone" /> | ||
| <Body text="[email protected]" className="footer-email" /> | ||
| <SocialIcons text="Social links" className="social-links" /> | ||
| </div> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the Footer component more reusable with props.
The component currently uses hardcoded values, making it less reusable. Consider making it more configurable through props.
-export const Footer = () => {
+export const Footer = ({
+ name,
+ phone,
+ email,
+ profileImage,
+ heading = "Let's talk"
+}) => {
return (
<div className="footer-section">
<div className="footer-image-box">
- <img src={placeholderFooterImage} alt="Footer profile image of Johanna Eriksson" className="footer-profile-image" />
+ <img src={profileImage} alt={`Footer profile image of ${name}`} className="footer-profile-image" />
</div>
<div className="footer-contact-info">
- <H2 heading="Let's talk" />
- <Body text="Johanna Eriksson" className="footer-name" />
- <Body text="+46(0)70-525 33 16" className="footer-phone" />
- <Body text="[email protected]" className="footer-email" />
+ <H2 heading={heading} />
+ <Body text={name} className="footer-name" />
+ <Body text={phone} className="footer-phone" />
+ <Body text={email} className="footer-email" />
<SocialIcons text="Social links" className="social-links" />
</div>
</div>
);
};
+
+Footer.propTypes = {
+ name: PropTypes.string.isRequired,
+ phone: PropTypes.string,
+ email: PropTypes.string,
+ profileImage: PropTypes.string.isRequired,
+ heading: PropTypes.string
+};Committable suggestion skipped: line range outside the PR's diff.
| <H3 heading="I am Johanna Eriksson" /> | ||
| <H1 heading="Frontend Developer" /> | ||
| <Body text="Johanna is an exceptional developer known for her innovative solutions and exceptional coding abilities. She creates user-friendly applications and solves complex issues with ease. Her drive for excellence makes her a valuable asset to any project and a standout in the technology field." /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider revising content structure and perspective.
A few suggestions for improvement:
- The description is written in third person, which feels impersonal for a portfolio. Consider writing in first person to create a more direct connection with visitors.
- Move the text content to a separate content file for easier maintenance.
Example implementation:
// src/content/hero.js
export const heroContent = {
name: "Johanna Eriksson",
title: "Frontend Developer",
description: "I am passionate about creating user-friendly applications and solving complex problems. With a focus on innovative solutions and clean code, I bring dedication and expertise to every project I undertake."
};Then update the component to use this content:
- <H3 heading="I am Johanna Eriksson" />
- <H1 heading="Frontend Developer" />
- <Body text="Johanna is an exceptional developer known for her innovative solutions and exceptional coding abilities. She creates user-friendly applications and solves complex issues with ease. Her drive for excellence makes her a valuable asset to any project and a standout in the technology field." />
+ <H3 heading={`I am ${heroContent.name}`} />
+ <H1 heading={heroContent.title} />
+ <Body text={heroContent.description} />| const cardsData = category === "project-card" ? projectsData.projects : [ | ||
| { | ||
| title: "Being Scared: I Love Code", | ||
| date: "July 28th", | ||
| content: "Writing code is less about 'just working'. Instead, it's much more about creativity.", | ||
| image: "https://via.placeholder.com/300" | ||
| }, | ||
| { | ||
| title: "My First Code Review", | ||
| date: "July 31st", | ||
| content: "It's similar to creative writing in that the author architecting a story must use an assortment of tools.", | ||
| image: "https://via.placeholder.com/300" | ||
| }, | ||
| { | ||
| title: "JavaScript 101", | ||
| date: "August 3rd", | ||
| content: "A way to satisfy a particular objective through an assortment of tools and components.", | ||
| image: "https://via.placeholder.com/300" | ||
| }, | ||
| { | ||
| title: "Being Scared: I Love Code", | ||
| date: "July 28th", | ||
| content: "Writing code is less about 'just working'. Instead, it's much more about creativity.", | ||
| image: "https://via.placeholder.com/300" | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move hardcoded article data to a separate file.
The hardcoded article data should be moved to a separate data file for better maintainability and consistency with how project data is handled.
Create a new file articles.json:
{
"articles": [
{
"id": "article-1",
"title": "Being Scared: I Love Code",
"date": "July 28th",
"content": "Writing code is less about 'just working'. Instead, it's much more about creativity.",
"image": "https://via.placeholder.com/300"
},
// ... other articles
]
}| @@ -0,0 +1,116 @@ | |||
| /* eslint-disable react/prop-types */ | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using PropTypes instead of disabling ESLint validation.
Rather than disabling prop-types validation globally, consider defining PropTypes for better type safety and documentation.
-/* eslint-disable react/prop-types */
+import PropTypes from 'prop-types';Committable suggestion skipped: line range outside the PR's diff.
| {cardsData.map((card, index) => | ||
| category === "project-card" ? ( | ||
| <ProjectCard | ||
| key={index} | ||
| name={card.name} | ||
| content={card.content} | ||
| image={card.image} | ||
| netlifyLink={card.netlify} | ||
| githubLink={card.github} | ||
| /> | ||
| ) : ( | ||
| <ArticleCard | ||
| key={index} | ||
| title={card.title} | ||
| date={card.date} | ||
| content={card.content} | ||
| image={card.image} | ||
| /> | ||
| ) | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using array indices as React keys.
Using array indices as keys can lead to performance issues and bugs with component state. Consider using unique identifiers from your data instead.
-key={index}
+key={category === "project-card" ? card.name : card.title}Also, consider adding loading and error states to handle data fetching scenarios.
Committable suggestion skipped: line range outside the PR's diff.
| const ArticleCard = ({ title, date, content, image }) => ( | ||
| <div className="article-card"> | ||
| <div | ||
| className="article-image" | ||
| style={{ | ||
| backgroundImage: `linear-gradient(0deg, rgba(253, 111, 0, 0.30), rgba(253, 111, 0, 0.30)), url(${image})`, | ||
| }} | ||
| ></div> | ||
| <div> | ||
| <p className="date">{date}</p> | ||
| <H3 heading={title} className="title" /> | ||
| <Body text={content} className="content" /> | ||
| <Button label="Read Article" variant="outlined" fullWidth /> | ||
| </div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Fix ArticleCard implementation issues.
Several issues need attention:
- The Read Article button is missing link functionality
- Date formatting should be standardized
- Gradient overlay code is duplicated from ProjectCard
-const ArticleCard = ({ title, date, content, image }) => (
+const ArticleCard = ({ title, date, content, image, link }) => (
// ...
- <Button label="Read Article" variant="outlined" fullWidth />
+ <Button label="Read Article" variant="outlined" fullWidth link={link} />Consider creating a shared gradient overlay component to reduce code duplication.
Committable suggestion skipped: line range outside the PR's diff.
| ## Dependency Installation & Startup Development Server | ||
|
|
||
| This project uses npm (Node Package Manager) to handle its dependencies and run the development server. | ||
|
|
||
| ```bash | ||
| npm i && code . && npm run dev | ||
| ``` | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve setup instructions for better clarity and reliability.
The current setup command combines multiple operations which might fail silently. Consider:
- Listing prerequisites (Node.js version, etc.)
- Separating commands and explaining each step
- Adding troubleshooting tips
Example structure:
## Prerequisites
- Node.js (v16 or higher)
- npm (v8 or higher)
- VS Code (optional)
## Setup
1. Install dependencies:
```bash
npm install- Start development server:
npm run dev
<!-- This is an auto-generated comment by CodeRabbit -->
…x to prevent image and contact info from stretching too far apart. Also add margin 0 auto to center the section, change justify-content from space-evenly to center, and add align-self: stretch.
|
Thanks Jennie for notifying me!
I've updated the CardSection.css by adding a max-width of 1200px, and I
also updated the Footer.css to fix the large gap issue there as well.
Kindest regards / Med vänlig hälsning,
*====================*
*Johanna Eriksson*
Schibsted Enterprise Tech
Västra Järnvägsgatan 21
105 17 Stockholm
***@***.*** ***@***.***>
…On Thu, 7 Nov 2024 at 20:05, Jennie Dalgren ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Great job with this project!
You have reach almost all requirements but there are some small things
that differs in your version from the design file.
Take a look at how you handle the spacing of your projects, because they
stretch out too much. And on a larger screen it would look like this:
image.png (view on web)
<https://github.com/user-attachments/assets/9dc5420d-2986-46b4-9112-42055ff4cd51>
—
Reply to this email directly, view it on GitHub
<#393 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKKOL7L3H3XNPJ6BMQZRJYLZ7O2YBAVCNFSM6AAAAABRDE7UQSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMRRHA4TINRRGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Great job on the portfolio! You have followed the design to the letter! 🌟👏
Some suggestions of improvements (also see detailed description in the separate comments)
- Break out the article/journey and skills data into separate json files for cleaner code and maintainability.
- The card section with projects and articles is cut off and not scrollable on mobile and tablet. Suggestions to update the css for cards-container so it allow horizontal scrolling.
There was a problem hiding this comment.
The card section with projects and articles (my journey) is cut off on does not seem to work on tablet and mobile. It is not scroll able. Suggestion is to add column and overflow-x:auto to the grid so it is scrollable. We did the same in our design project last week so I know it works :-)
.cards-container {
grid-auto-flow: column; /* Cards flow horizontally /
grid-template-rows: 1fr; / To ensure a single row layout /
overflow-x: auto; / Enables horizontal scrolling /*
}
.project-card, .article-card {
min-width: 250px; /* Sets minimum width for each card for scrollable layout /*
}
There was a problem hiding this comment.
Very good comments in the readme file! I really feel the struggle and I can relate to most of the problems regarding this project week. It is also good to have when looking back on what you have created and learned. 🌟
There was a problem hiding this comment.
Suggestion for improvement;
Break out the article content to a separate json file (as you have did with the projects) and import it to the card section component.
There was a problem hiding this comment.
Suggestion of improvement;
Add a skills.json file instead of hardcoding it here. Will make the code cleaner and easier to maintain.
…d webkit-box to article-card body-text to limit the number of lines to 3, and add ellisis (three dots) as text-overflow
…f text is longer.
|
Thank Matilda for your feedback!
I apologize for the misunderstanding regarding the cards. I believe I took
the instruction a bit too literally when you said we should try to copy the
design as much as possible. I somehow assumed that we would fix the cards
later when we had the correct data. I’ve now addressed this, and the cards
are scrollable :)
I’ve also restored the responsiveness for the tablet layout in the footer,
somehow, it got removed. I also corrected the screen size breakpoint, which
was set incorrectly for some reason.
Thanks again!
Kindest regards / Med vänlig hälsning,
*====================*
*Johanna Eriksson*
Schibsted Enterprise Tech
Västra Järnvägsgatan 21
105 17 Stockholm
***@***.*** ***@***.***>
…On Mon, 9 Dec 2024 at 11:41, Matilda Brunemalm ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Please make all your scrollable sections actually scrollable as it looks
broken now. Also, check how your page looks on all sizes (e.g. the footer
doesn't fit in tablet width).
Skarmavbild.2024-12-09.kl.11.39.48.png (view on web)
<https://github.com/user-attachments/assets/d4715f93-232c-41ab-ac2b-71978a8df74b> Skarmavbild.2024-12-09.kl.11.39.23.png
(view on web)
<https://github.com/user-attachments/assets/c8dfa2f5-3015-4474-a923-147441dde239>
—
Reply to this email directly, view it on GitHub
<#393 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKKOL7LGS7G5XIKFYHWK2JL2EVXT7AVCNFSM6AAAAABRDE7UQSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIOBYGQYTANBQGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Johannas Portfolio link
Summary by CodeRabbit
Release Notes
New Features
index.htmlfile as the main entry point for the application.Hero,Footer,CardSection, andTextSection, enhancing the portfolio's layout.Buttoncomponent for consistent button styling and functionality.SocialIconscomponent.projects.jsonfile for project data management.Improvements
Chores