-
Notifications
You must be signed in to change notification settings - Fork 24
Попыркина Мария, М3337 #19
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: master
Are you sure you want to change the base?
Попыркина Мария, М3337 #19
Conversation
|
This pull request is automatically deployed with Now. Latest deployment for this branch: https://task-5-git-fork-mashajuventus-maria-popyrkina-task-6.itmo-yandex.now.sh |
| "not op_mini all" | ||
| ], | ||
| "dependencies": { | ||
| "@types/classnames": "2.2.8", |
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.
npm install --save-dev @types/*
| height: 600px; | ||
| background-color: #e5eaf0; | ||
| margin: 0; | ||
| /*background-color: black;*/ |
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.
Не оставляй мёртвый код
| margin-right: 20px; | ||
| } | ||
|
|
||
| .inp { |
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.
Надо бы добавить немного творчества при именовании переменных)
| @@ -0,0 +1,27 @@ | |||
| import React, { Component } from 'react'; | |||
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.
Импорты можно делить на группы:
- Встроенные модули в браузер (таких пока нет) / батарейки Node.js
- Сторонние библиотеки
- JS импорты внутри проекта
- Импорт ресурсов (css, font, images)
| import { Header } from '../header/Header'; | ||
| import { Menu } from '../menu/Menu'; | ||
| import { LettersList } from '../lettersList/LettersList'; | ||
| import { ILetter } from '../letter/ILetter'; |
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.
Можно рядом держать файлик types.ts
| template: string; | ||
| } | ||
|
|
||
| export class Main extends Component<{}, IState> { |
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.
Можно так:
public readonly state: IState = {
isDark: false,
letters: [],
openId: -1,
template: ''
}| this.searchLetters = this.searchLetters.bind(this); | ||
| this.setLetters = this.setLetters.bind(this); | ||
| this.changeColor = this.changeColor.bind(this); | ||
| this.curMin = 10; |
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.
Можно вынести из конструктора в поле класса.
И назвать чуть понятнее :)
public static CUR_MIN = 10;| openId: -1, | ||
| template: '' | ||
| }; | ||
| this.letterAdded = this.letterAdded.bind(this); |
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.
Можно удалить т.к ты используешь стрелочные функции сохранённые в поля класса
| isDark: false, | ||
| letters: [], | ||
| openId: -1, | ||
| template: '' |
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.
Убирай мёртвый код
| openId={this.state.openId} | ||
| openLetters={this.openLetters} | ||
| markNotNew={this.markNotNew} | ||
| isDark={this.state.isDark} |
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.
Для того чтобы в дерево компонентов глубоко прокидывать пропсы можно использовать контексты
| <Menu /> | ||
| <LettersList | ||
| letterAdded={this.letterAdded} | ||
| letterChose={this.letterChose} |
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.
onLetterChose
| lettersS[i].isVisible = | ||
| lettersS[i].letterText.includes(template) || lettersS[i].sender.includes(template); | ||
| } | ||
| this.setState({ template: template, letters: lettersS }); |
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.
template: template -> template
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.
lettersS -> letters
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.
this.setState(oldState => {
// Your code
});| const lettersS = this.state.letters; | ||
| for (let i = 0; i < lettersS.length; i++) { | ||
| if (lettersS[i].id === id) { | ||
| lettersS[i].classS = "notNew"; |
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.
Держи в голове, что изменение чего-то внутри стейта может привести к сложновоспроизводимым багам
| delay += genDelay; | ||
| } | ||
| this.diff = delay; | ||
| console.log(delay); |
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.
Ата-та
|
|
||
| import styles from './Menu.module.css'; | ||
|
|
||
| export class Menu extends Component { |
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.
Попробуй функциональные компоненты, для начала в местах где есть только вёрстка без логики
| openId: number; | ||
| letters: ILetter[]; | ||
| openLetters: () => void; | ||
| markNotNew: (a: number) => void; |
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.
id or index
| allLettersChose={this.props.allLettersChose} | ||
| changeColor={this.props.changeColor} | ||
| /> | ||
| <ul className={styles.lettersList}>{code}</ul> |
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.
Внутри ul не может быть других детей кроме li
| return ( | ||
| <section className={darkClass}> | ||
| <LetterHeader | ||
| letterAdded={this.props.letterAdded} |
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.
const { lettersDeleted, allLettersChose } = this.props;| } | ||
|
|
||
| export class LetterHeader extends Component<IProps> { | ||
| public constructor(props: IProps) { |
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.
private markerRef = React.createRef();| private markerRef: RefObject<HTMLInputElement>; | ||
|
|
||
| public render() { | ||
| const sqBut = cn(styles.squareForButton, stylesHead.headerPart); |
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.
sqBut
| <input | ||
| ref={this.markerRef} | ||
| type="checkbox" | ||
| name="CCC" |
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.
???
| type="checkbox" | ||
| name="CCC" | ||
| className={styles.inputButton} | ||
| onClick={() => { |
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.
Можно вынести в отдельный обработчик в классе, никаких доп. аргументов ты в обработчики не передаёшь
| name="resendButton" | ||
| value="Переслать" | ||
| className={stylesHead.headerButtons} | ||
| onClick={() => this.props.letterAdded(0)} |
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.
Тоже можно вынести
| /> | ||
| </li> | ||
| <li className={stylesHead.headerPart}> | ||
| <input |
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.
button type="button"
| outline: none; | ||
| background-color: inherit; | ||
| font-weight: 500; | ||
| color: #cccccc; |
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.
Можно сокращать до #ccc
| }, // .bind(this) | ||
| 1000); | ||
| } else if (this.props.classS === 'delete') { | ||
| this.markerRef.current.className = styles.letter_delete; |
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.
Юзай classList
| import styles from './Letter.module.css'; | ||
|
|
||
| interface IProps { | ||
| classS: string; |
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.
cls, modifier
| <input | ||
| type="checkbox" | ||
| className={styles.inputButton} | ||
| onChange={() => this.props.letterChose(this.props.id)} |
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.
Постарайся сокращать использование lambda функций в render, это плохо влияет на производительность
| /*background-color: black;*/ | ||
| } | ||
|
|
||
| .selectButton:after { |
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.
Отделяй псевдоэлементы от псевдоклассов
::
| border-right: 2px solid; | ||
| border-bottom: 2px solid; | ||
| color: #000000; | ||
| -webkit-transform: scale(0.85) rotate(47deg) skewX(12deg); |
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.
Есть тулза, которая сама проставляет префиксы: Autoprefixer
| .letterDate { | ||
| float: right; | ||
| margin-right: 20px; | ||
| font-family: HelveticaNeueCyr-Medium; |
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.
Надо писать fallback
| } | ||
|
|
||
| .letterDate { | ||
| float: right; |
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.
Вот это древности)) Используй float только для обтекания
| src: url('./sourse/fonts/HelveticaNeueCyr-Medium.eot?#iefix') format('embedded-opentype'), | ||
| url('././sourse/fonts/HelveticaNeueCyr-Medium.svg#HelveticaNeueCyr-Medium') format('svg'), | ||
| url('././sourse/fonts/HelveticaNeueCyr-Medium.ttf') format('truetype'), | ||
| url('././sourse/fonts/HelveticaNeueCyr-Medium.woff') format('woff'), |
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.
Актуальны только woff2, woff
| <img | ||
| className={styles.yandexMail_picture} | ||
| alt="yandexMailPicture" | ||
| src={actualLogo} |
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.
Можно было прост isDark ? darkLogo : logo
| @@ -0,0 +1,52 @@ | |||
| const senders = ['Mom', 'Dad', 'Cat', 'Dog', 'Apple', 'Teacher', 'Homie', 'Mole', | |||
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.
tsx -> ts
Реализованы поиск по письмам и темная тема.