Skip to content

Conversation

@adaczo
Copy link

@adaczo adaczo commented Mar 28, 2020

What changed?

Screenshot 2020-03-28 at 14 37 18

@vercel
Copy link

vercel bot commented Mar 28, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/code4ro/taskforce-fe-components/n2n79imge
✅ Preview: https://taskforce-fe-components-git-fork-adaczo-filtered-list.code4ro.now.sh

import "./filtered-list-item.styles.scss";
import CaretSvg from "../../../images/icons/caret.svg";

export class FilteredListItem extends React.Component {
Copy link
Member

Choose a reason for hiding this comment

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

All components in here are function components. For the sake of consistency, could you please convert yours to function components as well?

Copy link
Author

Choose a reason for hiding this comment

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

Refactored FilteredListItem to use a function.
@surdu For FilteredList I need to have local state, do you recommend a different approach?

@aniri
Copy link
Member

aniri commented Mar 28, 2020

but issue #31 was already in progress. @surdu

@surdu
Copy link
Member

surdu commented Mar 28, 2020

but issue #31 was already in progress. @surdu

@adaczo might have miss-identified what this PR will fix. It looks like a slightly different component to me, or am I wrong ?

@aniri Nice catch! Sorry, missed that!

@adaczo
Copy link
Author

adaczo commented Mar 28, 2020

but issue #31 was already in progress. @surdu

@adaczo might have miss-identified what this PR will fix. It looks like a slightly different component to me, or am I wrong ?

@aniri Nice catch! Sorry, missed that!

Talked with @RaduCStefanescu and decided to seperate that ticket into 2 smaller reusable components that is api agnostic and reusable in different places. This PR addresses the list component.

@RaduCStefanescu
Copy link
Contributor

Heya! I talked to @adaczo privately and we decided we needed some small changes to this component, so he started from scratch.

};

FilteredList.propTypes = {
config: PropTypes.object,
Copy link
Member

Choose a reason for hiding this comment

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

I believe it will be better if we define this as a PropTypes.shape. Please see here for an example.

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.

Tabbed list

4 participants