Test task by romant094 · Pull Request #2 · twinscom/frontend-test-task

romant094 · 2019-02-03T19:33:50Z

Anton Romankov

- Now activity renders on button click - Added settings as global variable just for test, but it works :-) - Small changes in the interface

Activity block is now external component; Small changes in logycs and design; Added comment to filters.

src/App.js

EvgenyOrekhov

Не получается запустить:

React.Children.only expected to receive a single React element child.
▼ 26 stack frames were expanded.
invariant
node_modules/react/cjs/react.development.js:105
onlyChild [as only]
node_modules/react/cjs/react.development.js:1298
Router.render
node_modules/react-router/es/Router.js:118
finishClassComponent
node_modules/react-dom/cjs/react-dom.development.js:15128
updateClassComponent
node_modules/react-dom/cjs/react-dom.development.js:15083
beginWork
node_modules/react-dom/cjs/react-dom.development.js:15967
performUnitOfWork
node_modules/react-dom/cjs/react-dom.development.js:19093
workLoop
node_modules/react-dom/cjs/react-dom.development.js:19134
HTMLUnknownElement.callCallback
node_modules/react-dom/cjs/react-dom.development.js:147
invokeGuardedCallbackDev
node_modules/react-dom/cjs/react-dom.development.js:196
invokeGuardedCallback
node_modules/react-dom/cjs/react-dom.development.js:250
replayUnitOfWork
node_modules/react-dom/cjs/react-dom.development.js:18341
renderRoot
node_modules/react-dom/cjs/react-dom.development.js:19252
performWorkOnRoot
node_modules/react-dom/cjs/react-dom.development.js:20156
performWork
node_modules/react-dom/cjs/react-dom.development.js:20066
performSyncWork
node_modules/react-dom/cjs/react-dom.development.js:20040
requestWork
node_modules/react-dom/cjs/react-dom.development.js:19895
scheduleWork
node_modules/react-dom/cjs/react-dom.development.js:19702
scheduleRootUpdate
node_modules/react-dom/cjs/react-dom.development.js:20406
updateContainerAtExpirationTime
node_modules/react-dom/cjs/react-dom.development.js:20432
updateContainer
node_modules/react-dom/cjs/react-dom.development.js:20500
ReactRoot.push../node_modules/react-dom/cjs/react-dom.development.js.ReactRoot.render
node_modules/react-dom/cjs/react-dom.development.js:20813
(anonymous function)
node_modules/react-dom/cjs/react-dom.development.js:20967
unbatchedUpdates
node_modules/react-dom/cjs/react-dom.development.js:20283
legacyRenderSubtreeIntoContainer
node_modules/react-dom/cjs/react-dom.development.js:20963
render
node_modules/react-dom/cjs/react-dom.development.js:21030
▲ 26 stack frames were expanded.
Module../src/index.js
src/index.js:5
  2 | import ReactDOM from 'react-dom';
  3 | import App from './App';
  4 | 
> 5 | ReactDOM.render(<App />, document.getElementById('root'));
View compiled
__webpack_require__
/home/orehov/Downloads/frontend-test-task/webpack/bootstrap:782
  779 | };
  780 | 
  781 | // Execute the module function
> 782 | modules[moduleId].call(module.exports, module, module.exports, hotCreateRequire(moduleId));
      | ^  783 | 
  784 | // Flag the module as loaded
  785 | module.l = true;
View compiled
fn
/home/orehov/Downloads/frontend-test-task/webpack/bootstrap:150
  147 | 		);
  148 | 		hotCurrentParents = [];
  149 | 	}
> 150 | 	return __webpack_require__(request);
      | ^  151 | };
  152 | var ObjectFactory = function ObjectFactory(name) {
  153 | 	return {
View compiled
0
http://localhost:3000/static/js/main.chunk.js:977:18
__webpack_require__
/home/orehov/Downloads/frontend-test-task/webpack/bootstrap:782
  779 | };
  780 | 
  781 | // Execute the module function
> 782 | modules[moduleId].call(module.exports, module, module.exports, hotCreateRequire(moduleId));
      | ^  783 | 
  784 | // Flag the module as loaded
  785 | module.l = true;
View compiled
checkDeferredModules
/home/orehov/Downloads/frontend-test-task/webpack/bootstrap:45
  42 | 	}
  43 | 	if(fulfilled) {
  44 | 		deferredModules.splice(i--, 1);
> 45 | 		result = __webpack_require__(__webpack_require__.s = deferredModule[0]);
     | ^  46 | 	}
  47 | }
  48 | return result;
View compiled
Array.webpackJsonpCallback [as push]
/home/orehov/Downloads/frontend-test-task/webpack/bootstrap:32
  29 | 	deferredModules.push.apply(deferredModules, executeModules || []);
  30 | 
  31 | 	// run deferred modules when all chunks ready
> 32 | 	return checkDeferredModules();
     | ^  33 | };
  34 | function checkDeferredModules() {
  35 | 	var result;
View compiled
(anonymous function)
http://localhost:3000/static/js/main.chunk.js:1:57

romant094 · 2019-02-05T10:03:43Z

Не получается запустить

Исправил. Был неверный import

EvgenyOrekhov

С точки зрения пользователя есть два замечания по работе приложения:

поля ввода
- если стереть значения, поля не подсвечиваются красным и не блокируется кнопка "Filter"
- если в первое поле попытаться ввести a, перейти во второе поле и нажать b, то в первом поле появится a
модальное окно
- в тексте говорится "You found 5 activities", но фактически пользователь увидел только 4 активности
мобильный режим
- в интерфейсе "Filters" элементы расположены неэргономично

src/components/commonData.js

EvgenyOrekhov · 2019-02-05T12:18:08Z

src/components/Activities.js

+                            <button className='btn btn-small' onClick={modalClose}>Cancel</button>
+                        </div>
+                        <div className="col-1-of-2">
+                            <Link to="/filter/" className='btn btn-small'>Try!</Link>


Можно обойтись без глобальных CSS-классов, воспользовавшись свойством as у styled-components: https://www.styled-components.com/docs/basics#extending-styles (второй пример).

В случае с Button и Link можно сделать так:

const Button = styled.button` /* ...styles... */ `; const SmallButton = styled(Button)` /* ...more styles... */ `; <SmallButton>Cancel</SmallButton> <SmallButton as={Link} to="/filter/">Try!</SmallButton>

Окей, учту на будущее.

EvgenyOrekhov · 2019-02-05T12:36:53Z

src/components/commonData.js

+}
+
+export default GlobalStyle;
+export { Button, Headline, Container, ContainerInnerWrap, ActivityWrap, Strong, ActivityContent };


В идеале каждый модуль должен отвечать за и экспортировать что-то одно. Этот модуль можно разделить как минимум на три: GlobalStyle.js, StyledComponents.js, и ActivityContent.js.

EvgenyOrekhov · 2019-02-05T12:41:22Z

src/components/commonData.js

+const Price = styled.span`
+    font-size: 24px;
+`
+const ActivityContent = (props) => {


Стоит активнее использовать деструктуризацию, особенно в сигнатурах функций. props не говорит ни о чём, а взглянув на const ActivityContent = ({filter, data}) => { сразу становится понятно что в компонент нужно передать.

Исправил

data тоже было бы лучше деструктурировать:

const ActivityContent = ({filter, data}) => { const { activity, key, type /* ... */ } = data; return /* ... */; };

Опять же для повышения самодокументированности кода.

EvgenyOrekhov · 2019-02-05T13:30:02Z

src/components/commonData.js

+					</Price>
+				</PriceWrap>
+			</div>
+			<div className={Object.keys(props.data).length === 1 ? null : 'display-none'}>


Повторная проверка условия Object.keys(props.data).length === 1. Возможно было бы лучше воспользоваться вот этой техникой: https://reactjs.org/docs/conditional-rendering.html#inline-if-else-with-conditional-operator.

Исправил

Я имел в виду вместо

<div className={isError ? 'display-none' : ''}> <Success /> </div> <div className={isError ? '' : 'display-none'}> <Error /> </div>

сделать

{ isError ? <Error /> : <Success /> }

Исправил

Только теперь div-контейнеры в Success и Error (которые выполняют проверку на data.error) вообще не нужны, так как эта проверка выполняется уровнем выше, в ActivityWrap.

EvgenyOrekhov · 2019-02-05T13:43:56Z

src/components/Activities.js

+`
+function Activity() {
+    const [data, setData] = useState({});
+    let [clickCount, setClickCount] = useState(1);


Почему let, а не const?

Возможно копипаст... Исправил.

EvgenyOrekhov · 2019-02-05T13:50:31Z

src/components/Activities.js

+            <ContainerInnerWrap>
+                <Headline>WhattodoApp</Headline>
+                <PreambulaText>{Object.keys(data).length === 0 ? `Don't know what to do? We'll help you! Just click the button and you'll see the possible variant for your vocation...` : `Don't like that? Click... ;-)`}</PreambulaText>
+                <Button onClick={getRandomActivity} style={{ marginBottom: '20px' }}>Random</Button>


Для стилизации лучше использовать какой-то один способ. Если это styled-components, то все стили должны объявляться с помощью styled-components.

ОК, учту. Но как быть в том случае, если в одном месте одному элементу нужно задать отступ? Делать новый компонент по аналогии с этим?

src/components/filteredActivites.js

EvgenyOrekhov · 2019-02-05T14:06:06Z

src/components/filteredActivites.js

+            let url = 'http://www.boredapi.com/api/activity';
+
+            if (filterType === 'priceRange') {
+                url = `${url}?minprice=${firstValue}&maxprice=${secondValue}`;


Лучше использовать готовый инструмент для работы с query string, например https://www.npmjs.com/package/qs.

Окей, учту.

EvgenyOrekhov · 2019-02-06T08:10:36Z

Теперь если в первое поле попытаться ввести a, перейти во второе поле и нажать b, то в первом поле появится NaN.

romant094 · 2019-02-06T08:47:27Z

Теперь если в первое поле попытаться ввести a, перейти во второе поле и нажать b, то в первом поле появится NaN.

Если честно, с этим не знаю, что делать с этим...

EvgenyOrekhov · 2019-02-06T11:00:33Z

Теперь если в первое поле попытаться ввести a, перейти во второе поле и нажать b, то в первом поле появится NaN.

Если честно, с этим не знаю, что делать с этим...

Может быть в данном случае бОльшую часть валидации вообще стоит переложить на плечи браузера?

<input type="number" min="1" required>

romant094 and others added 12 commits January 30, 2019 00:03

Create CONTRIBUTORS.md

c0451fe

First commit

62c3f01

Added page with random activity

3883f96

Changes:

86e5532

- Now activity renders on button click - Added settings as global variable just for test, but it works :-) - Small changes in the interface

Added page with filters. Started iplemening logics.

711901f

Removed css, added it as a stiled component.

1e67702

Added logics to filters page

6855ae9

Renamed component, some changes in styles, changes on structure

4993aaa

Added validation to input fields;

c64e5e5

Activity block is now external component; Small changes in logycs and design; Added comment to filters.

Implemented UI for activity output

2c0d813

Added modal popup dependin on number of clicks; small changes in UI

f02496e

Cleared all comments, deleted useless code and unused imports

0532820

EvgenyOrekhov reviewed Feb 5, 2019

View reviewed changes

src/App.js Outdated Show resolved Hide resolved

EvgenyOrekhov requested changes Feb 5, 2019

View reviewed changes

romant094 added 3 commits February 5, 2019 12:55

fix: wrong dependicy

5675d2b

fix rename file (case)

bd77eca

fix: right case in name

101895a

romant094 added 2 commits February 5, 2019 13:07

fix

193802b

fix name conflicts

510d559

EvgenyOrekhov requested changes Feb 5, 2019

View reviewed changes

romant094 added 6 commits February 6, 2019 00:39

fix

f4ec02a

refactoring

24cf406

changed let to const

1581c68

Refactoring

4ba138c

refactoring

36c945e

Changed null on '' in className conditions

31c1701

romant094 added 2 commits February 6, 2019 11:19

refactoring

e966904

Added css reset module

a51a977

refactoring

7483fc6

Conversation

romant094 commented Feb 3, 2019

Uh oh!

Uh oh!

EvgenyOrekhov left a comment

Choose a reason for hiding this comment

Uh oh!

romant094 commented Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EvgenyOrekhov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EvgenyOrekhov commented Feb 6, 2019

Uh oh!

romant094 commented Feb 6, 2019

Uh oh!

EvgenyOrekhov commented Feb 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

romant094 commented Feb 5, 2019 •

edited

Loading