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

Failed event deliveries list #5163

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

Cloud11PL
Copy link
Member

@Cloud11PL Cloud11PL commented Sep 16, 2024

What type of PR is this?

  • 💅 Refactor
  • 🌟 Feature
  • 🔥 Bug Fix
  • 🔩 Maintenance
  • 🛠 Workflow CI/CD changes

Related Issues or Documents

  • closes #

Usage Instructions, Screenshots, Recordings

CleanShot 2024-09-23 at 13 11 37

App install loading state:
CleanShot 2024-09-23 at 11 59 27

Have you written tests?

  • Yes!
  • No... here is why: Writing tests are mandatory, please replace this text with why test are not included in this PR

[Optional] Description

Copy link

changeset-bot bot commented Sep 16, 2024

🦋 Changeset detected

Latest commit: 2ec5b38

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
saleor-dashboard Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot temporarily deployed to pr-5163 September 16, 2024 12:09 Destroyed
@github-actions github-actions bot temporarily deployed to pr-5163 September 16, 2024 12:47 Destroyed
@github-actions github-actions bot temporarily deployed to pr-5163 September 16, 2024 19:34 Destroyed
@github-actions github-actions bot temporarily deployed to pr-5163 September 17, 2024 11:36 Destroyed
@github-actions github-actions bot temporarily deployed to pr-5163 September 17, 2024 12:44 Destroyed
@Cloud11PL Cloud11PL marked this pull request as ready for review September 17, 2024 13:12
@Cloud11PL Cloud11PL requested a review from a team as a code owner September 17, 2024 13:12
@github-actions github-actions bot temporarily deployed to pr-5163 September 18, 2024 11:33 Destroyed
@github-actions github-actions bot temporarily deployed to pr-5163 September 18, 2024 14:53 Destroyed
poulch
poulch previously approved these changes Sep 18, 2024
andrzejewsky
andrzejewsky previously approved these changes Sep 23, 2024
Comment on lines +16 to +39
export const EventDeliveriesList: React.FC<EventDeliveriesListProps> = ({ eventDeliveries }) => (
<>
{eventDeliveries.map((ed, index) => {
const { createdAt, id } = ed.node;
const attempts = ed.node.attempts?.edges?.map(({ node }) => node) ?? [];
const attemptsCount = attempts.length;
const lastAttemptDate = attempts[attemptsCount - 1]?.createdAt;
const hasMore = index < eventDeliveries.length - 1;

return (
<EventDeliveryItem
key={id}
dataTestId={id}
createdAt={createdAt}
status={ed.node.status}
attemptsCount={attemptsCount}
attempts={attempts}
lastAttemptDate={lastAttemptDate}
hasMore={hasMore}
/>
);
})}
</>
);
Copy link
Member

Choose a reason for hiding this comment

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

In cases like this you could create something like DeliveryItem class or (even function + interfcace) and EventDeliveryItem component could take a single argument of this type.

That class can have static method eg. createFrom(deliveryEntry: DeliveryEntry) - in that case you have elegant separation and you are able to simply call:

{eventDeliveries.map(entry => <EventDeliveryItem  deliveryItem={DeliveryItem.from(entry)}  />)}

and this component is not needed anymore.

Think about that in the future, not needed now.

dataTestId?: string;
}

const MAX_ATTEMPTS = 6;
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to link some docs here so we know where this value come from

@github-actions github-actions bot temporarily deployed to pr-5163 September 23, 2024 14:01 Destroyed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants