Skip to content
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

hw_2_setupReact_code #1

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

hw_2_setupReact_code #1

wants to merge 16 commits into from

Conversation

orenkole
Copy link
Owner

No description provided.

@@ -0,0 +1,3 @@
node_modules
coverage
build-storybook.log

Choose a reason for hiding this comment

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

не указана парка сборки dist

Copy link
Owner Author

Choose a reason for hiding this comment

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

Спасибо, добавил

"prop-types": "^15.7.2",
"react": "^17.0.2",
"react-dom": "^17.0.2",
"typescript": "^4.4.4"

Choose a reason for hiding this comment

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

он явно не отсюда

Copy link
Owner Author

Choose a reason for hiding this comment

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

деинсталировал prop-types

Comment on lines 4 to 10
npx --no-install lint-staged
npx --no-install lint-staged
npx --no-install lint-staged
npx --no-install lint-staged
npx --no-install lint-staged
npx --no-install lint-staged
npx --no-install lint-staged

Choose a reason for hiding this comment

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

Зачем столько - вот пример настройки typicode/husky#949

tsconfig.json Outdated
"skipLibCheck": true, /* Skip type checking of declaration files. */
"forceConsistentCasingInFileNames": true, /* Disallow inconsistently-cased references to the same file. */
"downlevelIteration": true,
"types": ["@emotion/core"]

Choose a reason for hiding this comment

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

вот так TypeScript потерял преимуществ в типизации https://www.typescriptlang.org/tsconfig#types

Copy link
Owner Author

Choose a reason for hiding this comment

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

Спасибо, исправил

describe("Field", () => {
test("renders Field component", () => {
render(
<Field

Choose a reason for hiding this comment

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

Потерян колбек handleCellClick - из за отключения типизации этого не видно. В след тесте тоже.

TS2741: Property 'handleCellClick' is missing in type '{ sizes: { width: number; height: number; }; cells: { isChecked: false; serialNumber: number; id: number; }[]; }' but required in type 'FieldProps'.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Спасибо, исправил


export type FieldProps = {
sizes: {width: number; height: number},
cells: any,

Choose a reason for hiding this comment

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

надо избавлятся от ANY - подняв тип из Cell

Copy link
Owner Author

Choose a reason for hiding this comment

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

Спасибо, исправил

@@ -0,0 +1,13 @@
import React from "react";

const Row = (props: any) => {

Choose a reason for hiding this comment

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

Надо избавлятсья от Any - например через типизацию PropsWithChildren<{}>

Comment on lines +4 to +6
exports.PROJECT_ROOT = PROJECT_ROOT;
exports.BUILD_DIRECTORY = resolve(PROJECT_ROOT, './dist');
exports.SOURCE_DIRECTORY = resolve(PROJECT_ROOT, './src');

Choose a reason for hiding this comment

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

не очень понял зачем это, если даже в конфиге вебпака используется как константа, так и прямое вычисление директорий

Copy link
Owner Author

Choose a reason for hiding this comment

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

Спасибо, оставил

Comment on lines 19 to 22
{
test: /\.css$/,
use: ["style-loader", "css-loader"],
},

Choose a reason for hiding this comment

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

в проекте нет css - зачем нужен лодер ?

Copy link
Owner Author

@orenkole orenkole Nov 12, 2021

Choose a reason for hiding this comment

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

Спасибо, исправил

src/App.tsx Outdated

const handleCellClick = (serialNumber: number) => {
setCells((prevState) => {
console.log(" prev state ", prevState);

Choose a reason for hiding this comment

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

подчистить

Copy link
Owner Author

Choose a reason for hiding this comment

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

Подчистил

package.json Outdated
"test:watch": "jest --watch",
"start": "webpack serve --mode development --hot",
"build": "npx webpack --mode production",
"prepare": "husky install && npx husky add .husky/pre-commit \"npx --no-install lint-staged\"",

Choose a reason for hiding this comment

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

&& npx husky add .husky/pre-commit \"npx --no-install lint-staged\"",
Вот это нужно убрать, потому что prepare скрипт запускается при каждом запуске npm install, и вы каждый раз в файл дописываете еще один запуск lint-staged

Choose a reason for hiding this comment

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

Еще хук не имеет прав на запуск, установил chmod +x - заработало
lint-staged

Copy link
Owner Author

Choose a reason for hiding this comment

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

Спасибо, исправил

src/App.tsx Outdated
const App = () => {

const sizes = {width: 4, height: 2};
const initialCellsState = Array.from({ length: (sizes.height * sizes.width) }, (_, i) => ({

Choose a reason for hiding this comment

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

Я бы задал интерфейс для состояния

Copy link
Owner Author

Choose a reason for hiding this comment

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

Спасибо, добавил


const [cells, setCells] = useState(initialCellsState);

const handleCellClick = (serialNumber: number) => {
Copy link

@centralToNowhere centralToNowhere Nov 7, 2021

Choose a reason for hiding this comment

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

Возможно в данном задании это некритично, но при клике по одной ячейке перерисовывается все поле, если я не ошибаюсь.


const [cells, setCells] = useState(initialCellsState);

const handleCellClick = (serialNumber: number) => {
Copy link

Choose a reason for hiding this comment

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

для чего такой проброс handleClick из App в Сеll через Field? Почему не объявить его в Field? и сделать ему там же внутренний state

Copy link
Owner Author

@orenkole orenkole Nov 12, 2021

Choose a reason for hiding this comment

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

подумал, что эти параметры могут задаваться в каком-нибудь другом компоненте

@Vladimir2308
Copy link

Добрый день, можно дополнить readme с необходимыми коммандами для запуска

@@ -0,0 +1,47 @@
import React from "react";
import CSS from "csstype";

Choose a reason for hiding this comment

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

'CSS' is defined but never used

@Vladimir2308
Copy link

Если скачать код с гита, обновить зависимости, то storybook после этого не запускается.
Тесты проходят, проект запускается и функционирует

<Cell
key={index}
cell={props.cells[index]}
handleCellClick={props.handleCellClick}
Copy link

Choose a reason for hiding this comment

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

Если сделать поле 100х100 не много ли обработчиков onClick будут в DOM? Может выгоднее поставить один на родителя?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants