Skip to content

Comments

Project - Jonas Høier Christiansen#6

Open
Joe-HC wants to merge 2 commits intouvdata:masterfrom
Joe-HC:master
Open

Project - Jonas Høier Christiansen#6
Joe-HC wants to merge 2 commits intouvdata:masterfrom
Joe-HC:master

Conversation

@Joe-HC
Copy link

@Joe-HC Joe-HC commented Feb 25, 2019

My solution for the challenges and some additionally changes:

  • New list with the characters name for each movie
  • Various UI details added (icons, list header, loading text)
  • Dockerfile added

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.

Din beskrivelse virker meget lovende, desværre kan jeg ikke køre din projekt.

Mvh,
Wouter


EXPOSE 3000

COPY package.json package.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Mangler der måske også en yarn.lock?


const dataCached = getState().data[endpoint]

if (!dataCached || dataCached.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Vil det sige at du ikke stoler på cache, hvis den indeholder 0 items?

if (!dataCached || dataCached.length === 0) {
dispatch({ type: types.START_LOAD });

return loadData('https://swapi.co/api/' + endpoint + '/')
Copy link
Contributor

Choose a reason for hiding this comment

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

Overvej template string

}
</p>
<div className="btn-group">
<button disabled={loading} onClick={() => onChooseEndpoint('people')} className={"btn " + (endpoint === 'people' ? 'btn-success' : 'btn-primary')}><i className="fa fa-male"/><i className="fa fa-female"/> Characters</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

Læs evt her hvorfor arrow functions (eller bind) direkte i JSX sjældent er en god ide

https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md


COPY . /app

RUN yarn build
Copy link
Contributor

Choose a reason for hiding this comment

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

Jeg kan ikke bygge din docker image:

Step 7/8 : RUN yarn build
 ---> Running in 585b63465f34
yarn run v1.13.0
error Command "build" not found.

dispatch({ type: types.TOGGLE_EXPAND_ITEM, payload: item });
};

export const onMergeLists = (endpoint) => (dispatch, getState) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Jeg har lidt svært med at gennemskue meningen med denne action? Og da jeg ikke kan bygge, og dermed heller ikke kan køre, din docker image kan jeg heller ikke se/opleve hvad den gør i browseren.

}
<br />
{!this.props.data.films || !this.props.data.people
? <span>Hint: A secret button will appear when you have seen both the lists of characters and films</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Lidt sjov hemmelighed, hvis du afslører den med det samme 🤣

: <span>What do you want to see?</span>
}
<br />
{!this.props.data.films || !this.props.data.people
Copy link
Contributor

Choose a reason for hiding this comment

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

Jeg kan ikke se at componenten modtage en data prop ud fra PropTypes, er det en forglemmelse eller var det for at holde den hemmelige knap lidt mere hemmeligt.

...state,
data: {
...state.data,
[action.payload]: state.data.films.map(film => ({ ...film, charactersNames: film.characters.map(character => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Kan gøres nemmere at læse med lidt linebreaks de rigtige steder.

COPY package.json package.json
RUN yarn install

COPY . /app
Copy link
Contributor

Choose a reason for hiding this comment

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

Hvis jeg allerede har en node_modules liggende, vil den overskrive de node_modules som lige er blevet installeret på linje 8.

@woutervanvliet woutervanvliet self-assigned this Feb 25, 2019
@Joe-HC
Copy link
Author

Joe-HC commented Feb 26, 2019

Jeg havde lavet en forglemmelse - manglede at tilføje build-kommandoen under scripts i package-filen. Det er comittet nu.
Beklager mange gange.

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.

3 participants