Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15,573 changes: 10,349 additions & 5,224 deletions package-lock.json

Large diffs are not rendered by default.

8 changes: 7 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@
"@testing-library/react": "^13.4.0",
"@testing-library/user-event": "^13.5.0",
"react": "^18.2.0",
"react-challenge-interns": "file:",
"react-dom": "^18.2.0",
"react-scripts": "5.0.1",
"react-router-dom": "^6.26.2",
"react-scripts": "^5.0.1",
"web-vitals": "^2.1.4"
},
"scripts": {
Expand All @@ -34,5 +36,9 @@
"last 1 firefox version",
"last 1 safari version"
]
},
"devDependencies": {
"react-test-renderer": "^18.3.1",
"tailwindcss": "^3.4.13"
}
}
38 changes: 0 additions & 38 deletions src/App.css

This file was deleted.

25 changes: 0 additions & 25 deletions src/App.js

This file was deleted.

20 changes: 20 additions & 0 deletions src/App.jsx
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>
Comment on lines +9 to +14
Copy link
Copy Markdown

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:

import { NotFound } from './pages'

// ... existing routes ...
<Route path='*' element={<NotFound />} />

Make sure to create a NotFound component in your pages directory if it doesn't exist already.

<Footer/>
</div>
)
}
Comment on lines +5 to +18
Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested 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>
)
}
const App = () => {
return (
<>
<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/>
</>
)
}


export default App
8 changes: 0 additions & 8 deletions src/App.test.js

This file was deleted.

3 changes: 3 additions & 0 deletions src/assets/arrow.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions src/assets/calendar.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added src/assets/dotted-pattern.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
11 changes: 11 additions & 0 deletions src/assets/loader.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 4 additions & 0 deletions src/assets/location-grey.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 4 additions & 0 deletions src/assets/location.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
16 changes: 16 additions & 0 deletions src/assets/spinner.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
48 changes: 48 additions & 0 deletions src/components/ActorCard.jsx
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
Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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
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, actorId }) => {

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;
13 changes: 13 additions & 0 deletions src/components/Footer.jsx
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&copy; Betse95
</h3>
</section>
)
}

export default Footer
13 changes: 13 additions & 0 deletions src/components/Header.jsx
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;
25 changes: 25 additions & 0 deletions src/components/LinkSection.jsx
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
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import React from "react";
import arrow from "../assets/arrow.svg";
import { Link } from "react-router-dom";
const LinkSection = ({ lable, data }) => {
import React from "react";
import arrow from "../assets/arrow.svg";
import { Link } from "react-router-dom";
const LinkSection = ({ label, data }) => {

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>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<p className="p-bold-20 text-grey-600">{lable}: </p>
<p className="font-bold text-xl text-gray-600">{label}: </p>

{/* <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
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix issues in data mapping and Link component

There are several issues in the data mapping and Link component creation:

  1. Missing 'key' prop in the mapped elements.
  2. Incorrect OR operator in the 'to' prop.
  3. Extra curly brace in the 'to' prop template literal.
  4. Non-descriptive 'alt' text for the arrow image.

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:

  1. Add a 'key' prop to each Link component for better React performance.
  2. Fix the OR operator from bitwise (|) to logical (||).
  3. Remove the extra curly brace in the 'to' prop.
  4. Provide a more descriptive 'alt' text for the arrow image.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<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>
<div className="flex flex-wrap gap-6">
{data.map((data) => (
<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="Navigate to details" />
</Link>
))}
</div>
🧰 Tools
🪛 Biome

[error] 15-15: Missing key property for this element in iterable.

The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.

(lint/correctness/useJsxKeyInIterable)

</div>
);
};

export default LinkSection;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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:

npm install prop-types

or

yarn add prop-types

7 changes: 7 additions & 0 deletions src/components/index.js
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 };
66 changes: 57 additions & 9 deletions src/index.css
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;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Typo in font-family name: "Poppins" instead of "Poopins"

The font-family name "Poopins" appears to be misspelled. The correct spelling is "Poppins". This typo may prevent the font from loading correctly.

Apply this diff to fix the typo:

-  font-family: "Poopins", sans-serif;
+  font-family: "Poppins", sans-serif;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
font-family: "Poopins", sans-serif;
font-family: "Poppins", sans-serif;

}

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
Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 @apply to group shared styles.

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.

}
}
9 changes: 4 additions & 5 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,14 @@ import ReactDOM from 'react-dom/client';
import './index.css';
import App from './App';
import reportWebVitals from './reportWebVitals';
import { BrowserRouter as Router } from 'react-router-dom';

const root = ReactDOM.createRoot(document.getElementById('root'));
root.render(
<React.StrictMode>
<App />
<Router>
<App />
</Router>
</React.StrictMode>
);

// If you want to start measuring performance in your app, pass a function
// to log results (for example: reportWebVitals(console.log))
// or send to an analytics endpoint. Learn more: https://bit.ly/CRA-vitals
reportWebVitals();
1 change: 0 additions & 1 deletion src/logo.svg

This file was deleted.

Loading