-
Notifications
You must be signed in to change notification settings - Fork 18
Updated repo from betselot #6
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,20 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import React from 'react' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Route, Routes } from 'react-router-dom' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Actors, ActorDetails, Films, Starships } from './pages' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {Header, Footer} from './components' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const App = () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <div> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <Routes> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <Route path='/' element={<Actors/>}/> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <Route path='/actor/:id' element={<ActorDetails/>}/> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <Route path='/films/:id' element={<Films/>}/> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <Route path='/starships/:id' element={<Starships/>}/> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </Routes> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <Footer/> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+5
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Use React Fragment instead of div To avoid adding an unnecessary div to the DOM, consider using a React Fragment as the wrapper for your component's return statement. Apply this change: const App = () => {
return (
- <div>
+ <>
<Routes>
<Route path='/' element={<Actors/>}/>
<Route path='/actor/:id' element={<ActorDetails/>}/>
<Route path='/films/:id' element={<Films/>}/>
<Route path='/starships/:id' element={<Starships/>}/>
</Routes>
<Footer/>
- </div>
+ </>
)
}📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export default App | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,48 @@ | ||||||||||||||||||||||||||||
| import React from "react"; | ||||||||||||||||||||||||||||
| import { Link } from "react-router-dom"; | ||||||||||||||||||||||||||||
| import arrow from "../assets/arrow.svg"; | ||||||||||||||||||||||||||||
| import calendar from "../assets/calendar.svg"; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const ActorCard = ({ actor,url }) => { | ||||||||||||||||||||||||||||
| const id = url.split("/").filter(Boolean).pop(); // Getting the id of the actor from the url | ||||||||||||||||||||||||||||
|
Comment on lines
+1
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider passing the actor's ID directly as a prop. The current implementation extracts the actor's ID from the URL prop. While this works, it couples the component to a specific URL structure and may make it less reusable. Consider passing the actor's ID directly as a prop for better modularity and easier testing. You could modify the component props like this: -const ActorCard = ({ actor, url }) => {
- const id = url.split("/").filter(Boolean).pop(); // Getting the id of the actor from the url
+const ActorCard = ({ actor, actorId }) => {This change would require updating the parent component where ActorCard is used, but it would make the component more flexible and easier to maintain. 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||
| <div className="group relative flex min-h-[280px] w-full max-w-[300px] flex-col overflow-hidden rounded-xl bg-white shadow-md transition-all hover:shadow-lg md:min-h-[338px] m-5"> | ||||||||||||||||||||||||||||
| <div | ||||||||||||||||||||||||||||
| // style={{ backgroundImage: `url(${actor?.imageUrl})` }} // This can be used if there was an imageUrl in the actor object | ||||||||||||||||||||||||||||
| className="flex-center flex-grow bg-gray-50 bg-cover bg-center text-grey-500" | ||||||||||||||||||||||||||||
| ></div> | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| <div className="flex min-h-[130px] flex-col gap-3 p-5 md:gap-4"> | ||||||||||||||||||||||||||||
| <div className="flex gap-2 flex-between"> | ||||||||||||||||||||||||||||
| <span className="text-[14px] font-semibold leading-[20px] w-min rounded-full bg-green-100 px-4 py-1 text-green-60"> | ||||||||||||||||||||||||||||
| {`${actor?.height}CM`} | ||||||||||||||||||||||||||||
| </span> | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| <div className="flex w-max rounded-full bg-grey-500/10 px-1 py-1"> | ||||||||||||||||||||||||||||
| <img src={calendar} alt="calendar" className="w-4 h-4" /> | ||||||||||||||||||||||||||||
| <p className="text-[14px] font-semibold leading-[20px] text-grey-500 px-1 line-clamp-1 capitalize"> | ||||||||||||||||||||||||||||
| {actor?.birth_year} | ||||||||||||||||||||||||||||
| </p> | ||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| <p className="p-medium-16 md:p-medium-20 line-clamp-2 flex-1 text-black"> | ||||||||||||||||||||||||||||
| {actor?.name} | ||||||||||||||||||||||||||||
| </p> | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| <div className="flex-between w-full"> | ||||||||||||||||||||||||||||
| <p className="p-medium-14 md:p-medium-16 text-grey-600 line-clamp-1 capitalize"> | ||||||||||||||||||||||||||||
| {actor?.gender} | ||||||||||||||||||||||||||||
| </p> | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| <Link to={`/actor/${id}`} className="flex gap-2 "> {/* Link to the ActorDetails page */} | ||||||||||||||||||||||||||||
| <p className="text-primary-500 text-nowrap ">Actor's Details</p> | ||||||||||||||||||||||||||||
| <img src={arrow} alt="arrow" /> | ||||||||||||||||||||||||||||
| </Link> | ||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| export default ActorCard; | ||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| import React from 'react' | ||
|
|
||
| const Footer = () => { | ||
| return ( | ||
| <section className="bg-primary-50 bg-dotted-pattern bg-cover bg-center py-5 md:p-5"> | ||
| <h3 className="wrapper text-right sm:text-right"> | ||
| 2024© Betse95 | ||
| </h3> | ||
| </section> | ||
| ) | ||
| } | ||
|
|
||
| export default Footer |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| import React from "react"; | ||
|
|
||
| const Header = ({ title }) => { | ||
| return ( | ||
| <section className="bg-primary-50 bg-dotted-pattern bg-cover bg-center py-5 md:p-5"> | ||
| <h3 className="wrapper h3-bold text-center sm:text-left"> | ||
| Actors {title} | ||
| </h3> | ||
| </section> | ||
| ); | ||
| }; | ||
|
|
||
| export default Header; |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,25 @@ | ||||||||||||||||||||||||||||||||||||||||||
| import React from "react"; | ||||||||||||||||||||||||||||||||||||||||||
| import arrow from "../assets/arrow.svg"; | ||||||||||||||||||||||||||||||||||||||||||
| import { Link } from "react-router-dom"; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| const LinkSection = ({ lable, data }) => { | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1
to
+5
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix typo in prop name There's a typo in the prop name 'lable'. It should be 'label'. Please apply the following change: -const LinkSection = ({ lable, data }) => {
+const LinkSection = ({ label, data }) => {Make sure to update all occurrences of 'lable' to 'label' throughout the component. 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||||||||||||||
| <div className=" flex flex-col gap-3"> | ||||||||||||||||||||||||||||||||||||||||||
| <div className="flex flex-col gap-2"> | ||||||||||||||||||||||||||||||||||||||||||
| <p className="p-bold-20 text-grey-600">{lable}: </p> | ||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Use standard Tailwind classes for consistency The className 'p-bold-20' for the title paragraph doesn't appear to be a standard Tailwind class. Consider using standard Tailwind classes for better maintainability and consistency. You could replace it with something like: - <p className="p-bold-20 text-grey-600">{lable}: </p>
+ <p className="font-bold text-xl text-gray-600">{label}: </p>This assumes you want a bold, 20px (xl in Tailwind) font in gray. Adjust the classes as needed to match your design system. 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
| {/* <p className="p-medium-26 lg-p-regular-18">{edir?.description}</p> */} | ||||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| <div className="flex flex-wrap gap-6"> | ||||||||||||||||||||||||||||||||||||||||||
| {data.map((data) => ( | ||||||||||||||||||||||||||||||||||||||||||
| <Link to={`/${lable.toLowerCase()}/${data.title | data.name}}`} className="flex gap-2"> | ||||||||||||||||||||||||||||||||||||||||||
| <p className="text-primary-500 text-nowrap ">{data.title || data.name}</p> | ||||||||||||||||||||||||||||||||||||||||||
| <img src={arrow} alt="arrow" /> | ||||||||||||||||||||||||||||||||||||||||||
| </Link> | ||||||||||||||||||||||||||||||||||||||||||
| ))} | ||||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+13
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix issues in data mapping and Link component There are several issues in the data mapping and Link component creation:
Please apply the following changes: <div className="flex flex-wrap gap-6">
{data.map((data) => (
- <Link to={`/${lable.toLowerCase()}/${data.title | data.name}}`} className="flex gap-2">
+ <Link
+ key={data.title || data.name}
+ to={`/${label.toLowerCase()}/${data.title || data.name}`}
+ className="flex gap-2"
+ >
<p className="text-primary-500 text-nowrap ">{data.title || data.name}</p>
- <img src={arrow} alt="arrow" />
+ <img src={arrow} alt="Navigate to details" />
</Link>
))}
</div>These changes will:
📝 Committable suggestion
Suggested change
🧰 Tools🪛 Biome
|
||||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| export default LinkSection; | ||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add PropTypes for better type checking and documentation The component is missing PropTypes validation for its props. Adding PropTypes can help with type checking and serve as documentation for the component's API. Consider adding PropTypes as follows: import PropTypes from 'prop-types';
// ... component code ...
LinkSection.propTypes = {
label: PropTypes.string.isRequired,
data: PropTypes.arrayOf(
PropTypes.shape({
title: PropTypes.string,
name: PropTypes.string
})
).isRequired
};
export default LinkSection;Don't forget to install the 'prop-types' package if it's not already in your project: or |
||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| import Header from "./Header"; | ||
| import Footer from "./Footer"; | ||
| import ActorCard from "./ActorCard"; | ||
| import LinkSection from "./LinkSection"; | ||
|
|
||
|
|
||
| export { Header, Footer, ActorCard, LinkSection }; |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,13 +1,61 @@ | ||||||
| @tailwind base; | ||||||
| @tailwind components; | ||||||
| @tailwind utilities; | ||||||
|
|
||||||
| body { | ||||||
| margin: 0; | ||||||
| font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', 'Roboto', 'Oxygen', | ||||||
| 'Ubuntu', 'Cantarell', 'Fira Sans', 'Droid Sans', 'Helvetica Neue', | ||||||
| sans-serif; | ||||||
| -webkit-font-smoothing: antialiased; | ||||||
| -moz-osx-font-smoothing: grayscale; | ||||||
| font-family: "Poopins", sans-serif; | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo in font-family name: "Poppins" instead of "Poopins" The font-family name Apply this diff to fix the typo: - font-family: "Poopins", sans-serif;
+ font-family: "Poppins", sans-serif;📝 Committable suggestion
Suggested change
|
||||||
| } | ||||||
|
|
||||||
| code { | ||||||
| font-family: source-code-pro, Menlo, Monaco, Consolas, 'Courier New', | ||||||
| monospace; | ||||||
| @layer utilities { | ||||||
| .wrapper { | ||||||
| @apply max-w-7xl lg:mx-auto p-5 md:px-10 xl:px-0 w-full; | ||||||
| } | ||||||
|
|
||||||
| .flex-between { | ||||||
| @apply flex justify-between items-center; | ||||||
| } | ||||||
|
|
||||||
| .flex-center { | ||||||
| @apply flex justify-center items-center; | ||||||
| } | ||||||
|
|
||||||
| /* text styles*/ | ||||||
| /* 64 */ | ||||||
| .h1-bold { | ||||||
| @apply font-bold text-[40px] leading-[48px] lg:text-[48px] lg:leading-[60px] xl:text-[58px] xl:leading-[74px]; | ||||||
| } | ||||||
|
|
||||||
| /* 40 */ | ||||||
| .h2-bold { | ||||||
| @apply font-bold text-[32px] leading-[40px] lg:text-[36px] lg:leading-[44px] xl:text-[40px] xl:leading-[48px]; | ||||||
| } | ||||||
|
|
||||||
| /* 36 */ | ||||||
| .h3-bold { | ||||||
| @apply font-bold text-[28px] leading-[36px] md:text-[36px] md:leading-[44px]; | ||||||
| } | ||||||
|
|
||||||
| /* 20 */ | ||||||
| .p-bold-20 { | ||||||
| @apply font-bold text-[20px] leading-[30px] tracking-[2%]; | ||||||
| } | ||||||
| .p-medium-20 { | ||||||
| @apply text-[20px] font-medium leading-[30px]; | ||||||
| } | ||||||
|
|
||||||
| .p-regular-20 { | ||||||
| @apply text-[20px] font-normal leading-[30px] tracking-[2%]; | ||||||
| } | ||||||
|
|
||||||
| .p-medium-18 { | ||||||
| @apply text-[18px] font-medium leading-[28px]; | ||||||
| } | ||||||
|
|
||||||
| .p-medium-16 { | ||||||
| @apply text-[16px] font-medium leading-[24px]; | ||||||
| } | ||||||
|
|
||||||
| .p-medium-14 { | ||||||
| @apply text-[14px] font-medium leading-[20px]; | ||||||
|
Comment on lines
+24
to
+59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider refactoring typography classes to reduce duplication Many of the typography utility classes share common properties like font sizes and line heights. To improve maintainability and reduce code duplication, consider creating base classes or utilizing Tailwind CSS's For example, define base styles: /* Base font sizes */
.text-size-20 {
@apply text-[20px] leading-[30px];
}
.text-size-18 {
@apply text-[18px] leading-[28px];
}
.text-size-16 {
@apply text-[16px] leading-[24px];
}
.text-size-14 {
@apply text-[14px] leading-[20px];
}
/* Font weights */
.font-bold {
@apply font-bold;
}
.font-medium {
@apply font-medium;
}
.font-regular {
@apply font-normal;
}
/* Tracking */
.tracking-2 {
@apply tracking-[2%];
}Then, update your utility classes: .p-bold-20 {
@apply text-size-20 font-bold tracking-2;
}
.p-medium-20 {
@apply text-size-20 font-medium;
}
.p-regular-20 {
@apply text-size-20 font-regular tracking-2;
}
.p-medium-18 {
@apply text-size-18 font-medium;
}
/* ...and so on for other classes... */This approach enhances reusability and simplifies future style adjustments. |
||||||
| } | ||||||
| } | ||||||
This file was deleted.
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.
🛠️ Refactor suggestion
Consider adding a 404 route
Your routing setup looks good, but it might be beneficial to add a catch-all route to handle 404 errors for any undefined routes. This would improve the user experience by providing a friendly error page instead of a blank screen for invalid URLs.
Consider adding a NotFound component and a catch-all route like this:
Make sure to create a
NotFoundcomponent in yourpagesdirectory if it doesn't exist already.