Домашнее задание 1 #8

denisstasyev · 2019-05-08T10:41:26Z

При выполнении дз возник ряд вопросов с созданием спрайта для картинок. Не получается настроить css-sprite-loader. В configs/webpack.config.js в rules: я должен вставить шаблон для css файлов, но там уже стоит на cssRegex обработчик. Да и в коде я указываю background: url(...) из javascript (так как думаю, что изменять одно место лучше, чем писать 1000 css- классов (если отдельно под каждый эмодзи создавать класс со своим background)) и понятно, что css-sprite-loader надо как-то отдельно настраивать, как? Или что-то вместо него использовать?
В src/components/MessageList/Message/Message.js возник вопрос как аккуратно обрабатывать XSS угрозу, когда любой html можно отправить, тем самым можно поломать всё теоретически.

chexex

css спрайты нужны :)

chexex · 2019-05-10T11:35:21Z

src/store/reducers/user.js

+            return updateObject(state, {user: state.concat(
+                                {name: state.name, isAuthorized: state.isAuthorized})});
+    }*/
+  return state;


Предлагаю убрать лишние строки (закомментированные)

chexex · 2019-05-10T20:10:51Z

src/App.js

+          <Switch>
+            <Route exact path="/" component={ChatList} />
+            <Route path="/chat/:id" component={Chat} />
+            {/* <Route path="/register" component={Register} /> */}


Лишнее?

chexex · 2019-05-10T20:18:12Z

src/components/Auth/Auth.js

+    const result = Object.keys(this.state.loginForm).reduce((res, key) => {
+      res[key] = this.state.loginForm[key].value;
+      return res;
+    }, {});


Предложу рассмотреть метод entries у Object

const result = Object .entires(this.state.loginForm) .reduce((acc, [key, value]) => { acc[key] = value.value return acc }, {})

chexex · 2019-05-10T20:29:14Z

src/components/Auth/Auth.js

+    if (rule.minLength) {
+      isValid = value.trim().length >= rule.minLength && isValid;
+    }
+    return isValid;


Хочу обратить внимание, что такой checkValidity подойдет для понимания, как происходит валидация форм, однако в промышленной разработке необходимо будет предусмотреть больше параметров для валидации ввода (вне зависимости от того, будет ли использована сторонняя библиотека, например, formik или функционал формы будет реализован самостоятельно):

тип данных для ввода

разрешенные символы

длина ввода

управление пробелами

chexex · 2019-05-10T20:30:46Z

src/components/Auth/Auth.js

+    inputData.value = event.target.value;
+    inputData.valid = this.checkValidity(inputData.value, inputData.validation);
+
+    const invalid = Object.keys(newFormData).some(key => {


Здесь тоже предложу entries

chexex · 2019-05-10T21:03:36Z

src/components/Chat/MessageForm/ExtrasPanel/ExtrasPanel.js

+}
+
+const mapStateToProps = state => {
+  return {


... state => ({ ... })

chexex · 2019-05-10T21:04:44Z

src/components/Chat/MessageForm/ExtrasPanel/FileButton/FileButton.js

-import ImageFileButton from "../../../../static/Chat/MessageForm/FileButton/paperclip.svg";
+import * as actionTypes from "../../../../../store/actionTypes/actionTypes";
+
+import ImageFileButton from "../../../../../static/Chat/MessageForm/FileButton/paperclip.svg";


@martinkomitsky dothell выглядит так :)

@chexex Лучше абсолютные пути? Или лучше просто в components для каждого компонента создать папку? Не начнётся ли ад, так что вообще будет непонятно что с чем как взаимодействует?

Лично я предпочитаю абсолютные пути. Для этого в .env файле нужно добавить строку

NODE_PATH=src/,

а в модулях делать так:

import * as actionTypes from "store/actionTypes/actionTypes"

chexex · 2019-05-10T21:08:02Z

src/components/Chat/MessageForm/ExtrasPanel/LocationButton/LocationButton.js

+    }
+
+    function success(position) {
+      var text = `My location: (${position.coords.latitude}, ${


Возможно, будет лучше, если в рамках одного модуля не мешать старые и новые типы переменных (var, let, const)
А в рамках одной строки не использовать сигнатурные вещи es5 (тип var) и es6 (литеральные строки) :))

chexex · 2019-05-10T21:11:17Z

src/components/Chat/MessageForm/ExtrasPanel/LocationButton/LocationButton.js

+      alert("Unable to retrieve your location!");
+    }
+
+    navigator.geolocation.getCurrentPosition(success.bind(this), error);


В модулях выше методы класса были определены через константные переменные, можно и в этом компоненте поступить так же :)

@chexex Тот самый комментарий

Здесь я имел ввиду определение методов класса через константы (стрелочные функции), мне, почему-то, казалось, что они везде так были определены.

https://reactjs.org/docs/faq-functions.html

После просмотра всего кода понял, что мне казалось. Комментарий не актуален. Однако статья может быть полезной :)

chexex · 2019-05-10T21:11:56Z

src/components/Chat/MessageForm/ExtrasPanel/ReactionButton/ReactionButton.js

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


Позже продолжу отсюда

Вот в этом компоненте как раз непонятно, как src в CSS вынести для css-sprite-loader

denisstasyev · 2019-05-11T08:01:14Z

Спасибо, буду исправлять, Auth пока до ума не доводил

chexex · 2019-05-11T20:00:10Z

src/components/Chat/MessageForm/ExtrasPanel/ReactionButton/ReactionButton.js

+          ".png?sprite=sprite-reactions")}
+        alt={"Reaction " + this.props.name}
+      />
+    );


Что если вместо img использовать span для отображения бэкграунда?

Например, у нас может определен список изображений (смайлов в нашем случае). Тогда мы сможем в css файле определить их бэкграунд по имени изображения, а в компоненте динамически подставить имя класса.

// list with images file export default [ 'one', 'two', 'three', 'four', ... ] // some.css file :root { --basic-bg-size: 24px 24px; } one { background: url("some-url-one?sprite"); background-size: var(--basic-bg-size); } two { background: url("some-url-two?sprite"); background-size: var(--basic-bg-size); } three { background: url("some-url-three?sprite"); background-size: var(--basic-bg-size); } ... // in jsx render () { const images = importedImages .map((image, idx) => ( <span className={image} key={`${image}_${idx}`} onClick={...} onDoubleClick={...} > </span> )) return ( <div className="someContainerClass"> {images} </div> ) }

@chexex хорошо, переделаю так, просто изначально посчитал, что писать так CSS слишком избыточно и не особо красиво

chexex · 2019-05-11T20:01:23Z

src/components/Chat/MessageForm/ExtrasPanel/ReactionButton/ReactionButton.js

+}
+
+const mapDispatchToProps = dispatch => {
+  return {


const mapDispatchToProps = dispatch => ({...})

chexex · 2019-05-11T20:06:15Z

src/components/Chat/MessageForm/SendButton/SendButton.js

    );
  }
 }

-export default SendButton;
+const mapStateToProps = state => {
+  return {


... state => ({...})

chexex · 2019-05-11T20:06:45Z

src/components/Chat/MessageForm/SendButton/SendButton.js

+};
+
+const mapDispatchToProps = dispatch => {
+  return {


... dispatch => ({ ... })

chexex · 2019-05-11T20:09:16Z

src/components/Chat/MessageList/Message/Message.js

@@ -1,79 +1,141 @@
 /* eslint-disable jsx-a11y/img-redundant-alt */
 import React from "react";
 import "./Message.css";
+// import "../../ReactionTypes/ReactionTypes.css";


Больше не нужно?

chexex · 2019-05-11T20:39:28Z

src/components/Chat/MessageList/Message/Message.js

+          '" style="max-width:100%;" alt="Reaction template" />'.replace(
+            "template",
+            reaction.name
+          );


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

resutl = `<img src="${src} style="max-width: 100%;" alt="Reaction ${reaction.name}" />`

chexex · 2019-05-11T20:43:41Z

src/components/Chat/MessageList/Message/Message.js

+            reaction.name
+          );
+      }
+      while (result.indexOf(reaction.text) !== -1) {


Почему здесь этот цикл, для чего он? :)

chexex · 2019-05-11T20:46:24Z

src/components/Chat/MessageList/Message/Message.js

-      <li className={"message " + (this.props.isMy ? "my" : "alien")}>
-        <div className={"text--" + (this.props.isMy ? "my" : "alien")}>
-          {text}
+          </div>


Много повторяющегося кода

chexex · 2019-05-11T20:47:17Z

src/components/Chat/MessageList/MessageList.js

-  }
-
-  onDragOver(event) {
+  handleDragAndDrop(event) {
    event.preventDefault();
    var files = Array.from(event.dataTransfer.files);

    files.forEach(function(file) {
      var reader = new FileReader();


От var необходимо избавиться

chexex · 2019-05-11T20:49:11Z

src/components/Chat/ReactionTypes.js

@@ -0,0 +1,64 @@
+const reactionTypesList = [


А вот и список с картинками. :) Для них можно завести правила в css, а в jsx подставлять только className соответсвтующий имени картинки

chexex · 2019-05-11T20:54:41Z

При выполнении дз возник ряд вопросов с созданием спрайта для картинок. Не получается настроить css-sprite-loader. В configs/webpack.config.js в rules: я должен вставить шаблон для css файлов, но там уже стоит на cssRegex обработчик. Да и в коде я указываю background: url(...) из javascript (так как думаю, что изменять одно место лучше, чем писать 1000 css- классов (если отдельно под каждый эмодзи создавать класс со своим background)) и понятно, что css-sprite-loader надо как-то отдельно настраивать, как? Или что-то вместо него использовать?

В src/components/MessageList/Message/Message.js возник вопрос как аккуратно обрабатывать XSS угрозу, когда любой html можно отправить, тем самым можно поломать всё теоретически.

Спасибо за проделанную работу. Я постарался ответить на поставленные вопросы по ходу ревью.
Если остались непонятные моменты, пожалуйста, задавай вопросы.

denisstasyev · 2019-05-13T18:52:47Z

@chexex

Исправил большинство замечаний, кроме одного комментария (отметил его) (Auth пока не трогал, далее им займусь как раз).
Никак не получается отображение через span сделать в Message в handleReaction(text) . В чём моя ошибка?
В config/webpack.config.js не знаю как аккуратно добавить строки для css-sprite-loader, так как там есть cssRegex и насколько я понимаю красиво было бы css-sprite-loader сюда же добавить, пробовал в use: ["css-sprite-loader", getStyleLoaders(...)], но ругается. --- сделано

denisstasyev · 2019-05-14T13:35:10Z

@chexex Добавил css-sprite-loader, но отображает он неверно иконки. Мне кажется, что
background-size: contain;
background-clip: initial;
могли бы исправить ситуацию, но их css-sprite-loader не поддерживает тут issue

chexex · 2019-05-15T16:33:20Z

src/components/Chat/MessageList/Message/Message.js

+          `<span className="${[
+            styles["reaction-inline"],
+            styles[reaction.name]
+          ].join(" ")}" />`


Здесь не получается отобразить?

Если честно, меня по-прежнему смущает этот алгоритм замены текстового представления смайлика :).
Я бы следовал одному из пути, указанному здесь:
https://stackoverflow.com/a/55334886

Теперь, чтобы наверняка понять, почему не работает, я прошу показать, как выглядит готовый кусок html кода. Возможно @martinkomitsky с ходу назовет проблему, но я в любом случае рекомендую приложить html/скрин

@chexex Вот тут плохо в ExtrasPanel отображаются

При отправке только смайла (то есть текста ": alien :" без пробелов)

при отправке :alien :123

chexex · 2019-05-15T16:36:10Z

src/components/Chat/MessageForm/ExtrasPanel/LocationButton/LocationButton.js

+      alert("Unable to retrieve your location!");
+    }
+
+    navigator.geolocation.getCurrentPosition(success.bind(this), error);


Здесь я имел ввиду определение методов класса через константы (стрелочные функции), мне, почему-то, казалось, что они везде так были определены.

https://reactjs.org/docs/faq-functions.html

После просмотра всего кода понял, что мне казалось. Комментарий не актуален. Однако статья может быть полезной :)

chexex · 2019-05-15T16:41:05Z

На будущее попрошу действия, подобные eject (автоматически генерирующие файлы, которые должны версионироваться), делать отдельным коммитом, иначе сложно ревьюить.

В общем и целом, я не вижу проблем в том, чтобы принять работу, жду следующие задания.

denisstasyev · 2019-05-15T16:59:59Z

@chexex Есть ещё вопрос как сделать, чтобы чат автоматически скролился вниз ?

Ну и по сдаче: вы можете тогда замержить (Мартин обычно сам делал это) или мне самому?

martinkomitsky · 2019-05-19T07:43:45Z

Коммент от Леши: мерж и 6 баллов.
@chexex Есть ещё вопрос как сделать, чтобы чат автоматически скролился вниз ?

denisstasyev added 2 commits May 8, 2019 12:15

Reactions added

f265a30

Eject + css-sprite-loader

5a82a0c

denisstasyev assigned martinkomitsky May 8, 2019

chexex reviewed May 10, 2019

View reviewed changes

chexex reviewed May 11, 2019

View reviewed changes

Changed import + prepared for css-sprite-loader

634a857

css-sprite-loader added

8d07da2

chexex reviewed May 15, 2019

View reviewed changes

denisstasyev assigned martinkomitsky and unassigned martinkomitsky May 15, 2019

martinkomitsky approved these changes May 19, 2019

View reviewed changes

martinkomitsky merged commit 76f68b8 into master May 19, 2019

martinkomitsky deleted the sem2-hw1 branch May 19, 2019 07:44

Домашнее задание 1 #8

Домашнее задание 1 #8

Conversation

denisstasyev commented May 8, 2019

chexex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denisstasyev May 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denisstasyev commented May 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chexex commented May 11, 2019

denisstasyev commented May 13, 2019 • edited Loading

denisstasyev commented May 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denisstasyev May 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chexex commented May 15, 2019

denisstasyev commented May 15, 2019

martinkomitsky commented May 19, 2019

denisstasyev May 11, 2019 •

edited

Loading

denisstasyev commented May 13, 2019 •

edited

Loading

denisstasyev commented May 14, 2019 •

edited

Loading

denisstasyev May 15, 2019 •

edited

Loading