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

ui: refine code and error handle #662

Merged
merged 10 commits into from
Jul 13, 2020

Conversation

baurine
Copy link
Collaborator

@baurine baurine commented Jun 30, 2020

close #594, close #595, close #596, close #602

What did:

Some other screenshots:

企业微信20200630012826

企业微信20200630012854

I planned to move Alert out of CardTable, then I found it doesn't work because we need to show alert between the table title and table body. If we move the Alert out of the CardTable, then it looks like this:

企业微信20200630011837

@baurine baurine marked this pull request as draft June 30, 2020 05:39
{errMessages.map(
(msg, idx) =>
msg && <Alert key={idx} message={msg} type="error" showIcon />
)}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to move the Alert of the CardTable but found it doesn't work because the Alert displays out of the table title which is not expected.

@baurine baurine marked this pull request as ready for review June 30, 2020 06:29
@baurine baurine mentioned this pull request Jun 30, 2020
const errMessages = useMemo(() => {
let errMsgs: string[] = []
if (errTiDB) {
errMsgs.push('Load TiDB instances failed')
Copy link
Member

@breezewish breezewish Jul 6, 2020

Choose a reason for hiding this comment

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

We'd better provide detailed error information. Keep the same error message as current one is not verbose enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think so.

Comment on lines 219 to 222
{errMessages.map(
(msg, idx) =>
msg && <Alert key={idx} message={msg} type="error" showIcon />
)}
Copy link
Member

Choose a reason for hiding this comment

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

you don't need to try to keep the same looking as the old one. You can refine and find a way that the code is simple and the user can know about the error.

@breezewish
Copy link
Member

I planned to move Alert out of CardTable, then I found it doesn't work because we need to show alert between the table title and table body

How about not displaying the table totally (and substitute it with the error bar)?

Copy link
Member

@breezewish breezewish left a comment

Choose a reason for hiding this comment

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

Thanks! Mostly fine, is it possible to remove apiErrorMsg and encapsulate it in a component, something like

<ErrorBar error={error} />

so that using it can be simple?

@baurine
Copy link
Collaborator Author

baurine commented Jul 6, 2020

Thanks! Mostly fine, is it possible to remove apiErrorMsg and encapsulate it in a component, something like

<ErrorBar error={error} />

so that using it can be simple?

Good idea, will try it later.

@breezewish
Copy link
Member

Is this PR reviewable now? If not, you can add a WIP :)

@baurine
Copy link
Collaborator Author

baurine commented Jul 13, 2020

Yep, just ready.

How about this:

  • Use a list to show multiple errors (show at most 3 kinds of errors):

    企业微信20200713015141

  • Use the original style for a single error:

    image

I planned to move Alert out of CardTable, then I found it doesn't work because we need to show alert between the table title and table body

How about not displaying the table totally (and substitute it with the error bar)?

The key point is that in the overview page, the statements and slow queries table have a table title shows above the alert and table body:

企业微信20200713015341

If we hide the whole table, the title will be hidden as well, which looks not good. Except that we re-implement the title by an extra Card component.

Copy link
Member

@breezewish breezewish left a comment

Choose a reason for hiding this comment

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

Good job!

@ti-srebot
Copy link

@breeswish,Thanks for your review.

@breezewish breezewish merged commit 5af71fe into pingcap:master Jul 13, 2020
breezewish added a commit that referenced this pull request Jul 15, 2020
* ui: Fix TiFlash log searching (#680)
* ui: Add a whitelist in i18next to fix the issue #675 (#677)
* docs: add ericsyh as a contributor (#681)
* ui: refine code and error handle (#662)
* *: Rename disable_telemetry to enable_telemetry (#684)
* ui: Update space behaviour in statement / slow log search (#682)
* ui: No need to generate ui library (#687)
* forwarder: evict invalid current picked remote (#689)
* keyviz: support to merge cold logical range (#674)
* Update release version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants