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

implement filtered list #120

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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?

src/components/filtered-list/filtered-list.js Outdated Show resolved Hide resolved
@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