Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: display specific errors if templates page fails#4023

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
presleyp merged 10 commits intomainfrompresleyp/template-errors
Sep 13, 2022

Conversation

presleyp
Copy link
Contributor

I didn't have a ticket for this one, but I noticed while working on this page that it was swallowing errors.

kylecarbs reacted with eyes emoji
@presleyppresleyp requested a review froma team as acode ownerSeptember 12, 2022 17:28
@presleyppresleyp requested review fromKira-Pilot and removed request fora teamSeptember 12, 2022 17:28
defaultMessage={t("errors.getTemplatesError")}
/>
) :(
<TableContainer>
Copy link
Member

Choose a reason for hiding this comment

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

I find this triple, JSX ternary a little confusing and personally think this is a good candidate for early return.

jsjoeio reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

How would you do the early return? I'm not seeing a good way of doing it since the margins and page header need to be there in each case. I could put those in a const but at that point I'd rather put the content (error or data) in a const and still use the conditional so you can easily see what's definitely there vs what's conditional.

Copy link
Member

Choose a reason for hiding this comment

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

I might move the margins and page header intoTemplatesPage.tsx - they seem like page scaffolding to me. Then, one could decompose theTemplatesPageView file:

export const TemplatesPageView = () => {  if (props.getOrganizationsError) {    return <ErrorSummary />  }  if (props.getTemplatesError) {    return <ErrorSummary />  }  if (empty) {    return (      <TableContainer>        <LoadingTableBody />      </TableContainer>    )  }  return (    <TableContainer>      <TemplateTableBody templates={templates} />    </TableContainer>  )}

I think the benefit here is we are writing state in a declarative way - it is apparent at a glance what states the developer should be aware of. Let me know what you think!

jsjoeio reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah, this looks nice and readable, but the scaffolding is in the view for storybook reasons. I'm open to a refactor but I think I'll leave it for future work so I can get the error handling in.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good!

</TableHead>
<TableBody>
{props.loading&&<TableLoader/>}
{empty&&(
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about making a parent table component that accepts a child table body component? That child component could be a loading component, or a data-filled component. I was wondering if breaking apart state into components here might make this a little easier to read and reason about.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Sounds like a good idea, can you make a ticket for it? We could apply it in multiple places.

import{TemplatesPageView}from"./TemplatesPageView"

constTemplatesPage:React.FC=()=>{
exportconstTemplatesPage:React.FC=()=>{
Copy link
Member

@Kira-PilotKira-PilotSep 12, 2022
edited
Loading

Choose a reason for hiding this comment

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

I think we need to update the import ofTemplatesPage inAppRouter.tsx and inTemplatesPage.test.tsx.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Gah, thanks!

<divclassName={styles.templateIconWrapper}>
<imgalt=""src={template.icon}/>
</div>
) :undefined
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need a ternary here.

Copy link
Member

@Kira-PilotKira-Pilot left a comment

Choose a reason for hiding this comment

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

This looks good after we fix the import issue!

@presleyppresleyp merged commit83c35bb intomainSep 13, 2022
@presleyppresleyp deleted the presleyp/template-errors branchSeptember 13, 2022 14:27
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@Kira-PilotKira-PilotKira-Pilot approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@presleyp@Kira-Pilot

[8]ページ先頭

©2009-2025 Movatter.jp