YANA_SENIUK-w1-React #8
Conversation
There was a problem hiding this comment.
Hi Yana,
Wow, there's a lot going on here! Tailwind and TS are advanced tools. I love them, and use them every day. But they are not always appropriate for every application. So I'm curious: why have you used them in this project? And why have you used bun as well?
Looking forward to hearing about your thought process.
| @@ -0,0 +1,27 @@ | |||
| { | |||
There was a problem hiding this comment.
Can you explain what is the difference between tsconfig.app.json, tsconfig.json, and tsconfig.node.json and why you need all three?
There was a problem hiding this comment.
Vite created them. Tsconfig.json for ts config (like module or not, I want to use .ts or without, which version of JS and other rules).
ts.app - for front-end and tsconfig.node.json for back-end. In my case 1 is enough, but I just didn't tough what Vite created.
There was a problem hiding this comment.
@yanaesher That's not quite right - the node file is used for vite's own compiler which runs locally, not for a backend.
It is okay to use more advanced tools like this, but make sure you understand the code you're committing. Otherwise, when things go wrong, you will find it very difficult to understand how to fix it.
https://stackoverflow.com/a/72030801/13063136
| @@ -0,0 +1 @@ | |||
| /// <reference types="vite/client" /> | |||
There was a problem hiding this comment.
Can you explain why you need this file?
There was a problem hiding this comment.
For TS to recognize Vite's types
| } | ||
|
|
||
| @layer utilities { | ||
| .products-list > :nth-child(5n + 1) { |
There was a problem hiding this comment.
This works for a layout when you have 5 items across, but what about mobile layouts? 🟠
| } | ||
|
|
||
| .category:hover, | ||
| .category:focus { |
There was a problem hiding this comment.
Nice attention to detail, particularly including :focus for accessibility.
| .category.selected { | ||
| background-color: var(--color-bg-selected); | ||
| color: var(--color-text-selected); | ||
| transition: background-color 0.3s ease; |
There was a problem hiding this comment.
It's important to be consistent so that other coders can understand and work with your code. Either use @apply everywhere you can or set css properties instead of applying tailwind util classes. This should be a rule for all of your css files, not just this line. 🔵
week1/project/ecommerce/src/App.tsx
Outdated
|
|
||
| export function App() { | ||
| const [selectedCategory, setSelectedCategory] = useState<Category | "">( | ||
| () => (localStorage.getItem("selectedCategory") as Category) || "" |
There was a problem hiding this comment.
Are you sure you need to cast the type here?
There was a problem hiding this comment.
No, I did a mistake. LocalStorage just a string =((
There was a problem hiding this comment.
Plus I have generic here <Category | "">, so no...
| @@ -0,0 +1,4 @@ | |||
| import type { Category } from "../types/category"; | |||
|
|
|||
| export const normalizeCategory = (category: Category): string => | |||
| @@ -0,0 +1 @@ | |||
| export type Category = string; // leave it if categories from backend | |||
There was a problem hiding this comment.
Making custom types is good practice in TypeScript. However, if your custom type is simply a basic string then it's not really adding any value. You should either a) delete this type and use string where you've used Category, or b) make this into a union type or enum so that the TS compiler knows it's limited to certain values.
There was a problem hiding this comment.
I thought if more categories come from Backend it will not work cause of Enums
There was a problem hiding this comment.
I admire your full-stack thinking, but you should also build your software to suit the stack you're actually using. You don't have a backend, and this weekly assignment will never have a backend. So we should not code with backend in mind.
|
|
||
| export function ProductsSection({ products }: ProductsPanelProps) { | ||
| return ( | ||
| <section className="products py-6" aria-label="Products list"> |
There was a problem hiding this comment.
Is there a .products class? Is this doing anything?
There was a problem hiding this comment.
no just for easier reading and understanding or better only add class if I want to do some styling
There was a problem hiding this comment.
Better to only add classes if you want to do some styling or need to target that element(s) for other reasons.
| selectedCategory, | ||
| }: CategoryListProps) { | ||
| return ( | ||
| <ul className="categories-list flex flex-wrap gap-2"> |
There was a problem hiding this comment.
I think categories-list is another class that doesn't exist in your css. Be sure to keep your code clean and only use code that is actually functional. Please check all classNames on all your JSX to make sure they're useful. 🟠
Cannot enter Netlify or other resourses to deploy.