Skip to content
This repository was archived by the owner on Mar 19, 2021. It is now read-only.

Conversation

@sergeysova
Copy link
Member

@sergeysova sergeysova commented Jun 27, 2019

Closes #69
Closes #68

@sergeysova sergeysova requested review from avetalia, mg901 and stuneak June 27, 2019 20:29
@avetalia
Copy link
Contributor

Looks elegant

list={cards}
renderExists={(list) => (
<CardsItemsBlock>
{list.filter(Boolean).map((card) =>
Copy link

Choose a reason for hiding this comment

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

list.map(card => Boolean(card) && render...

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case you will have array with empty elements

Copy link

Choose a reason for hiding this comment

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

Array of false and needed elements, so what?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is eager optimization

export const SkeletonList = ({
isLoading,
ids,
count = 3,
Copy link

Choose a reason for hiding this comment

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

magic numbers. Need to move to named const

<CardsList
key={`card-view-${current.id}`}
ids={[current.id]}
renderCard={({ card, onUsefulClick }) => (
Copy link

Choose a reason for hiding this comment

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

That function can be static (defined in the upper scope)

}}
>
Favorite
<ZeroTab active={tab === "created"} onClick={() => setTab("created")}>
Copy link

Choose a reason for hiding this comment

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

All string literals ("useful", "created") must be defined statically in one place and reused by imports for safety feature changes

Copy link
Member Author

@sergeysova sergeysova Jun 28, 2019

Choose a reason for hiding this comment

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

This is not a final solution. After a redesign, I will return to this code

@sergeysova sergeysova merged commit 337ecd2 into dev Jun 29, 2019
@sergeysova sergeysova deleted the fix/no-cards-loaded branch June 29, 2019 17:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Home page don't load cards when open direct Card don't opens when open direct link

5 participants