Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ctl 05 Create a card component #6

Merged
merged 15 commits into from Sep 11, 2019

Conversation

@JBudny
Copy link
Owner

commented Aug 25, 2019

No description provided.

@JBudny JBudny requested review from IdaszakDaniel and MHekert Aug 25, 2019
@JBudny JBudny changed the title Ctl 05 Ctl 05 Create a card component Aug 25, 2019
@@ -1,19 +1,21 @@
import Typography from "@material-ui/core/Typography";
import React from "react";
import PropTypes from "prop-types";
import { useSelector } from "react-redux";
import Card from "./tab_content/Card/Card";

This comment has been minimized.

Copy link
@IdaszakDaniel

IdaszakDaniel Aug 25, 2019

Collaborator

use camelCase throughout whole codebase!

@@ -0,0 +1,160 @@
// @flow
import "../../../../utils/fontello/css/fontello.css";
import { isEqual } from "lodash";

This comment has been minimized.

Copy link
@IdaszakDaniel

IdaszakDaniel Aug 25, 2019

Collaborator

you can import from "lodash/isEqual" if it is the only one thing in component

setDislikeHeart(!dislikeHeart);
};

return (

This comment has been minimized.

Copy link
@IdaszakDaniel

IdaszakDaniel Aug 25, 2019

Collaborator

you can split this component into one with business logic and nested one with rendered interface. Think if It's possible to split this into smaller components to allow reusability eg. CardWrapper, Card, LikeButtons.

This comment has been minimized.

Copy link
@JBudny

JBudny Aug 31, 2019

Author Owner

I've splitted this component into smaller ones (interfaces) and moved their logic (functions and states) directly where they are used because it was problematic to pass them into deeply nested components as well as testing their impact on the parent component state. Please check if it's acceptable, I can still move them back to the CardContainer.

Right now their logic is inside ...interface\BottomPanel.js and ...interface\TitlePanel.js files

import React from "react";
import Card from "./Card";

const dummyPost = {

This comment has been minimized.

Copy link
@IdaszakDaniel

IdaszakDaniel Aug 25, 2019

Collaborator

export this one, It could be useful when working with different tests

}
`;

exports[`Card Component should match the snapshot 1`] = `

This comment has been minimized.

Copy link
@IdaszakDaniel

IdaszakDaniel Aug 25, 2019

Collaborator

check if you can improve this snapshot. It can lead to false positives due to some numbers possibly rendered from the library

const dispatch = useDispatch();

useEffect(() => {
const getModes = quantity => dispatch(getModesByDateInitial(quantity));

This comment has been minimized.

Copy link
@IdaszakDaniel

IdaszakDaniel Aug 25, 2019

Collaborator

Is this function necessary? The only idea I have is to reuse it somewhere, but It's declared in local scope. Either export it or remove

<span>404 Not Found</span>
<hr />
<p>
The page &#34;

This comment has been minimized.

Copy link
@IdaszakDaniel

IdaszakDaniel Aug 25, 2019

Collaborator

Export this as a function to the file with labels. It's important for using different languages in the app or just for the minor changes in the text displayed inside app.

This comment has been minimized.

Copy link
@JBudny

JBudny Aug 31, 2019

Author Owner

I feel like it's already a little too much changes on this branch. I'll do this as another task along with intl implementation.

@JBudny JBudny requested a review from IdaszakDaniel Aug 31, 2019
@@ -0,0 +1,15 @@
const dummyProcessedPost = {
tags: `non eiusmod, cupidatat sit, ex, sint, in`,

This comment has been minimized.

Copy link
@MHekert

MHekert Aug 31, 2019

Collaborator

You could import dummyPost from src/utils/dummyCardProps/dummyPost and use it's properties and overwrite tags property to avoid changing it in multiple places if API response structure changes.

const CardContainer = (props: Props) => {
const { post } = props;

const processedPost = JSON.parse(JSON.stringify(post));

This comment has been minimized.

Copy link
@MHekert

MHekert Aug 31, 2019

Collaborator

You could use spread operator here.

const [dislikeHeart, setDislikeHeart] = useState(false);

const handleLikeClick = () => {
if (dislikeHeart) setDislikeHeart(!dislikeHeart);

This comment has been minimized.

Copy link
@MHekert

MHekert Aug 31, 2019

Collaborator

You could extract this as a another function to not repeat yourself.

const [dislikeHeart, setDislikeHeart] = useState(false);

const handleLikeClick = () => {
if (dislikeHeart) setDislikeHeart(!dislikeHeart);

This comment has been minimized.

Copy link
@MHekert

MHekert Aug 31, 2019

Collaborator

You could write simply: if (dislikeHeart) setDislikeHeart(false); to make it more readable.

@JBudny JBudny requested review from MHekert and removed request for MHekert and IdaszakDaniel Sep 2, 2019
@JBudny JBudny requested a review from MHekert Sep 10, 2019
Copy link
Collaborator

left a comment

lgtm


beforeEach(() => (bottomPanelTree = mount(<BottomPanel />)));

const selectedNodesEqualTo = (sel, val) =>

This comment has been minimized.

Copy link
@MHekert

MHekert Sep 11, 2019

Collaborator

You could move this declaration to the top of block.

@JBudny JBudny merged commit 8d8c7f1 into develop Sep 11, 2019
@JBudny JBudny deleted the CTL-05 branch Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.