Assignment from UVdata#4
Assignment from UVdata#4CamillaHorne wants to merge 7 commits intouvdata:masterfrom CamillaHorne:master
Conversation
- Added planets, species, starships, and vehicles - Collapse/Expand all buttons - Order by keys extracted from the schema - Added the Dockerfile
woutervanvliet
left a comment
woutervanvliet
left a comment
There was a problem hiding this comment.
Hej Camilla,
Tak for besvarelse. Jeg har efterladt en del kommentar som du evt lige kan kigge på.
Men overordnet set fint løsning, den virker (næsten) og du er kommet godt rundt i både React og Redux.
Mvh,
Wouter
Dockerfile
Outdated
| @@ -0,0 +1,12 @@ | |||
| FROM node:8 | |||
There was a problem hiding this comment.
Hvorfor præcis node 8, og ikke en noget nyere version?
Der mangler også en EXPOSE, som angiver hvad for en port applikationen kører på.
| dispatch({ type: types.START_LOAD }); | ||
|
|
||
| function isCached() { | ||
| var current_state = getState(); |
There was a problem hiding this comment.
Har du overvejet at bruge const?
swapi/actions.js
Outdated
| console.error(err); | ||
| alert('It went wrong.') | ||
| }) | ||
| .then(() => loadSchema(url)) |
There was a problem hiding this comment.
Godt fundet, det schema der ;)
Men er der en særlig grund til at du venter med at hente schema'et, indtil data er hentet? Kunne man ikke gøre det samtidig?
swapi/actions.js
Outdated
| var current_state = getState(); | ||
|
|
||
| var data = [].concat(current_state.data[current_state.endpoint]); | ||
| data.sort((a, b) => (a[selectedKey].toString()).localeCompare(b[selectedKey].toString())); |
There was a problem hiding this comment.
Der mangler lige at argument til localeCompare som angiver hvad for en locale du gerne vil bruge.
swapi/components/item-display.js
Outdated
|
|
||
| return (<span> | ||
| <button className="btn btn-xs btn-default" onClick={this.handleExpandToggle} aria-label={isExpanded ? 'Collapse' : 'Expand'}> | ||
| <button disabled={loading} className="btn btn-xs btn-default" onClick={() => onExpandToggle(this)} aria-label={isExpanded ? 'Collapse' : 'Expand'}> |
There was a problem hiding this comment.
Det her virker lidt underligt. Du kan læse her (https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md) hvorfor arrow functions direkte i JSX oftest ikke er en særlig god idé. Jeg er heller ikke så vildt med at sende component instanser "ud" af React.
| const expandAll = action.expandAll; | ||
|
|
||
| var data = state.data[state.endpoint]; | ||
| data = data.map((item) => { |
There was a problem hiding this comment.
På den her måde bliver hvert item i listen ændret, som gør at React vil rerender alle items. Du kunne vælge kun at lave et nyt object for det item som man expander, og bare returnere dem som dermed ikke bliver ændret.
Men en lidt smartere løsning kunne være ikke at gemme isExpanded på selve item, men i en object eller liste for sig. Din store kunne således ser sådan ud:
{
data: {
// data som den er nu
},
expandedItemStatus: {
[itemUrl]: true,
[otherItemUrl]: false,
},
Dockerfile
Outdated
|
|
||
| COPY . /usr/src/app | ||
|
|
||
| ENV PATH /usr/src/app/node_modules/.bin:$PATH |
There was a problem hiding this comment.
Var den nødvendigt?
swapi/store.js
Outdated
|
|
||
| var data = state.data[state.endpoint]; | ||
| data = data.map((item) => { | ||
| if (itemDisplay !== undefined && item.url === itemDisplay.props.children.url) { |
There was a problem hiding this comment.
Som jeg henvist til tidligere, er det oftest ikke nogen god idé at tilgå React components udenfor React, da Redux ikke har noget med React at gøre - og dens actions og reducers også gerne skal kunne bruges med en anden UI library.
swapi/actions.js
Outdated
| var current_state = getState(); | ||
|
|
||
| var data = [].concat(current_state.data[current_state.endpoint]); | ||
| data.sort((a, b) => (a[selectedKey].toString()).localeCompare(b[selectedKey].toString())); |
There was a problem hiding this comment.
Når jeg vælger "Order by" fra listen hvor man vælger hvad der skal sorteres på, bliver der smidt denne fejl:
Uncaught TypeError: Cannot read property 'toString' of undefined
at actions.js?b04e09a:104
at Array.sort (<anonymous>)
at actions.js?b04e09a:103
at index.js?65fe2a4:10
at bindActionCreators.js?70a9bf2:5
at onChange (millennium-falcon.js?65fe2a4:50)
at HTMLUnknownElement.callCallback (react-dom.development.js?6f78f63:149)
at Object.invokeGuardedCallbackDev (react-dom.development.js?6f78f63:199)
at invokeGuardedCallback (react-dom.development.js?6f78f63:256)
at invokeGuardedCallbackAndCatchFirstError (react-dom.development.js?6f78f63:270)
swapi/actions.js
Outdated
| const sleep = (seconds, withValue) => new Promise(resolve => setTimeout(resolve, seconds * 1000, withValue)); | ||
|
|
||
| export const onChooseEndpoint = (endpoint) => (dispatch, getState) => { | ||
| const url = 'https://swapi.co/api/' + endpoint + '/'; |
There was a problem hiding this comment.
Har du overvejet at bruge en template string i stedet for concatenation?
…the value is null or undefined.
|
Jeg har nu set dine kommentare igennem og lavet nogle ændringer baseret på dem. |
|
@CamillaHorne Tak for dine ændringer, vi snakker videre om det på onsdag. |
3 participants
Finished the challenges and added some additional changes: