React Homework-嘉義練習生 Johnny Yu by johnnyyu0930 · Pull Request #3 · clonn/parcel-react-app

johnnyyu0930 · 2019-06-02T08:51:15Z

Todo list
movie search

clonn

感覺對於 react 已經有熟悉度，
對於套件有基本使用觀念。

可以思考一下如何將 component 拆解，讓每個元件的關係更為獨立。

clonn · 2019-06-03T16:33:21Z

README.md

-and you can build with no-minify or no-cache
+or

+$ yan run build


OH 打太快了＠＠是yarn

clonn · 2019-06-03T16:34:19Z

package.json

+    "react": "16.x",
+    "react-dom": "^16.8.0",
+    "react-router-dom": "^5.0.0",
+    "regenerator-runtime": "^0.13.2"


這個部分是？

之前 npm start 的時候一直跳出找不到regenerator-runtime
然後我就重裝就正常了不知道什麼原因

剛剛把它拿掉又可以動了～

可能因為是採用 yarn 安裝的關係，兩者機制上會不太一樣。

clonn · 2019-06-03T16:36:15Z

src/app.js

+        isloader: false,
+        isGet: true
+      })
+    }   


如果這部分要處理例外處理，建議將 if 判斷，改採用 try catch 的方式，

src/app.js

clonn · 2019-06-03T16:45:27Z

src/components/Movie.js

+    const src = `https://www.imdb.com/title/${id}`;
+
+    return (
+        <tr  onClick={(e) => openSite(e, src)}>


這邊用 onClick 的部分，採用 window.open , 這樣可能需要考慮會被瀏覽器預設開新視窗擋住。

建議還是用只是將 a 的範圍，延伸到整個 tr 區塊即可。

剛剛試過用a包 tr React 會跳warning

clonn · 2019-06-03T16:47:27Z

src/components/SearchBar.js

+import Button from '@material-ui/core/Button';
+
+const SearchBar = ({ ...props }) => {
+    const [keyword, setKeyword] = React.useState(props.keyword);


React.useState(props.keyword);

會建議直接改採用

import {useState} from "react"; // 這邊已經將 ...props, 理論上可以直接使用 keyword, 如果怕遇到問題可以這樣用， const [keyword, setKeyword] = React.useState(keyword || '');

// 這邊已經將 ...props, 理論上可以直接使用 keyword, 如果怕遇到問題可以這樣用

我試過直接使用keyword 他會找不到
發現他是把其他屬性放進去 props裡面

可以確認一下一開始進入的時候, 最外層所帶入的 props 到底代表什麼物件，這樣會耕清楚為何會導致 useState 會出現問題喔～

有時間可以試試看

clonn · 2019-06-03T16:50:43Z

src/containers/MovieList.js

+const MovieList = ({ movies, ...props }) => {
+    return (
+        <div>
+            <SearchBar {...props}/>


建議將 SearchBar 與 MovieList 獨立開來

兩者基本上是可以視為獨立元件，這樣也可以讓事件都是獨立傳送， SearchBar 才不會跟 movie list 的耦合度太高

了解謝謝

johnnyyu0930 · 2019-06-04T02:59:21Z

code review後已經把修改部分pull request

johnnyyu0930 added 8 commits May 29, 2019 12:36

todos add li

ca877da

todo list done

466df95

movieList done

f3c8812

readme update

4142e43

readme update

80bc2a7

add movie search bar and search function

5733ad0

readme update

3516d33

fix npm run build script

95d880f

clonn reviewed Jun 3, 2019

View reviewed changes

code review fix

bdce509

Conversation

johnnyyu0930 commented Jun 2, 2019

Uh oh!

clonn left a comment

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!

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!

johnnyyu0930 Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

johnnyyu0930 commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

johnnyyu0930 Jun 4, 2019 •

edited

Loading

johnnyyu0930 commented Jun 4, 2019 •

edited

Loading