Skip to content

Feature/issue#2_redux_arquitecture#7

Open
Carchuli wants to merge 8 commits intodevelopfrom
feature/issue#2-redux-arquitecture
Open

Feature/issue#2_redux_arquitecture#7
Carchuli wants to merge 8 commits intodevelopfrom
feature/issue#2-redux-arquitecture

Conversation

@Carchuli
Copy link
Copy Markdown
Collaborator

@Carchuli Carchuli commented Apr 7, 2020

Prepare a redux architecture example.

@dailymp
Copy link
Copy Markdown
Owner

dailymp commented Apr 8, 2020

Prepare a redux architecture example.

@Carchuli The PR sould be from your branch to develop branch is the correct way of working

@Carchuli Carchuli changed the base branch from master to develop April 8, 2020 12:16
dailymp
dailymp requested changes Apr 8, 2020
View reviewed changes
Copy link
Copy Markdown
Owner

@dailymp dailymp left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The PR must be from your branch to develop

00 Base/src/components/myComponent/myComponent.tsx Outdated
const { nameFromProps } = props;
const dispatch = useDispatch();
const fruitList = useSelector((state) => state.fruits.fruitList);
console.log('fruitList', fruitList);
Copy link
Copy Markdown
Owner

@dailymp dailymp Apr 8, 2020

Choose a reason for hiding this comment

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

Remove console.log

00 Base/src/components/myComponent/myComponent.tsx Outdated
deleteFruit,
addFruit,
} from './../../redux/actions/fruitsActions';
import './styles.scss';
Copy link
Copy Markdown
Owner

@dailymp dailymp Apr 8, 2020

Choose a reason for hiding this comment

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

Can you use less?

00 Base/src/components/myComponent/myComponent.tsx Outdated
fetchfruits,
deleteFruit,
addFruit,
} from './../../redux/actions/fruitsActions';
Copy link
Copy Markdown
Owner

@dailymp dailymp Apr 8, 2020

Choose a reason for hiding this comment

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

Can you use barrel pattern?

@Carchuli
Copy link
Copy Markdown
Collaborator Author

Carchuli commented Apr 8, 2020

The PR must be from your branch to develop

I've added some dependencies as material-ui, so you'll need to run: npm install to install them.
Also bugs have been fixed

@dailymp
Copy link
Copy Markdown
Owner

dailymp commented Apr 8, 2020 •
edited
Loading

The PR must be from your branch to develop

I've added some dependencies as material-ui, so you'll need to run: npm install to install them.
Also bugs have been fixed

Ok, I will review new changes you have commit, now I can see everything properly, it was problems with my git I have to clone again the repo because with git pull the new changes were not coming at all

dailymp
dailymp requested changes Apr 10, 2020
View reviewed changes
00 Base/src/redux/actions/fruitsActions.tsx
@@ -0,0 +1,28 @@
import { GET_ALL_FRUITS, DELETE_FRUIT, ADD_FRUIT } from './../constants';
import { getListOfFruit } from '../../myApi/index';
Copy link
Copy Markdown
Owner

@dailymp dailymp Apr 10, 2020

Choose a reason for hiding this comment

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

Please could you simulate api requests using axios with this fake api: https://jsonplaceholder.typicode.com/?

@dailymp
Copy link
Copy Markdown
Owner

dailymp commented Apr 10, 2020 •
edited
Loading

@Carchuli Please you have conflicts fix it!

@Carchuli
Copy link
Copy Markdown
Collaborator Author

Carchuli commented Apr 10, 2020

@dailymp
Copy link
Copy Markdown
Owner

dailymp commented Apr 13, 2020 •
edited
Loading

In this PR I can see you just left some files with conflicts, please @Carchuli can you fix it?

  • On the other hand I missing the other PR with tests? Do you have any problems to complete it?

@Carchuli Carchuli changed the title Feature/issue#2 redux arquitecture Feature/issue#2_redux_arquitecture Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dailymp dailymp dailymp requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Carchuli @dailymp