Skip to content

Comments

Assignment - Christian Fæster#5

Open
cfaester wants to merge 19 commits intouvdata:masterfrom
cfaester:master
Open

Assignment - Christian Fæster#5
cfaester wants to merge 19 commits intouvdata:masterfrom
cfaester:master

Conversation

@cfaester
Copy link

@cfaester cfaester commented Feb 25, 2019

Hello there.
- Obi-Wan Kenobi

This completes the four key challenges, while adhering to the code style employed. Also includes a Dockerfile that runs the application on-top of PM2 for nice log and process management.
Further, it adds a cookie-clicker esque game where you collect credits, buy characters and ships, to ultimately, generate even more credits. Details are included in the readme.

Looking forward to discussing this with you.

Copy link
Contributor

@woutervanvliet woutervanvliet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Det ser meget lovende ud, baseret på din beskrivelse. Men jeg får en fejl når jeg forsøger at kører din app.

Kan du kigge på det? Tak!

TypeError: Cannot read property 'endpoint' of null
    at reducer (http://localhost:3000/_next/1551113643306/page/index.js:11055:38)
    at p (<anonymous>:1:36402)
    at v (<anonymous>:1:36684)
    at <anonymous>:1:40069
    at Object.dispatch (http://localhost:3000/_next/1551113643306/page/index.js:8853:22)
    at e (<anonymous>:1:40553)
    at http://localhost:3000/_next/1551113643306/page/index.js:11196:16
    at dispatch (http://localhost:3000/_next/1551113643306/page/index.js:10745:18)
    at http://localhost:3000/_next/1551113643306/page/index.js:8213:3
    at Object.dispatch (http://localhost:3000/_next/1551113643306/page/index.js:11193:18)

@cfaester
Copy link
Author

cfaester commented Feb 25, 2019

@woutervanvliet
Jeg kigger på det. I midlertidig skulle et reload fikse problemet.

@cfaester
Copy link
Author

@woutervanvliet
Så skulle det køre igen. Serveren er også opdateret.

Copy link
Contributor

@woutervanvliet woutervanvliet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Jeg har tilføjet en del kommentar, som du evt kan kigge på. Men jeg kan godt lide din løsning.

Wouter

Dockerfile Outdated
RUN yarn install --production
RUN npm build

EXPOSE 80 443 43554 3000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hvorfor alle de porte?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PM2 binder dens webmonitorering til 43554. 80 og 443 er lidt ligegyldige eftersom jeg regner med en nginx/iis alligevel ville samle 3000 op i en anden container/på hosten.

}

.crew-chooser {
width: "fit-content";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Det er vist ikke helt sådan fit-content fungerer. Tror jeg

}
};

export const onSortListData = (endpoint, downDirection = true) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Du mener vel descending ;) ?

dispatch,
getState
) => {
if (!endpoint) return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hvornår vil dette sker?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Sort data" knappen kan altid trykkes på, selvom der ikke er loadet data ind. Og så er ekstra error checking altid at foretrække i mine øjne.

swapi/actions.js Outdated
) => {
if (!endpoint) return;
const list = [...getState().data[endpoint]];
if (!list) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Selv en tom array er "truthy", så denne condition er aldrig true


renderOwnedEntities() {
const { ownedEntities } = this.props;
let uniquePeople = countUniques(ownedEntities.people); // We are all unique :)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

countUniques implicerer lidt at den returnerer en number, men jeg kan se at uniquePeople bliver brugt som object.

Overvej i øvrigt også, om det ikke burde være in en mapStateToProps function denne beregning burde ligge.


function countUniques(list) {
let uniques = {};
list.map(ent => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

brug .forEach i stedet for .map

};

componentDidMount() {
setInterval(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

man burde cleare sådan en interval i componentWillUnmount

update() {
this.props.onGenerateMoney();
localStorage.setItem('redux-money', this.props.money);
document.title = `${this.props.money} credits - ${this.defaultTitle}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/nfl/react-helmet kan anbefales til formålet

swapi/store.js Outdated
};
}

case types.LOAD_FROM_LOCALSTORAGE: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redux er vel lidt ligeglad med hvor data kommer fra? Jeg vil forvente at en sådan action vil hedde SET_MONEY, fx.

@woutervanvliet woutervanvliet self-assigned this Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants