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

Improve error handling #251

Merged
merged 15 commits into from
Sep 11, 2022
Merged

Improve error handling #251

merged 15 commits into from
Sep 11, 2022

Conversation

0x5bfa
Copy link
Owner

@0x5bfa 0x5bfa commented Sep 3, 2022

⏱️ Before you start

  • Have you checked if a similar PR has already been requested?
  • Have you built and ran the app?

↗️ Related/Fixed issues

None

📄 Description

As the title

💭 Motivation and Context

  • Improve exception handling
  • Aggregate all async tasks into one async task so that they can be caught wherever exceptions occur.
  • Add new exception display page
  • Add new exception details dialog

Applied on:

  • User pages
  • Repository pages
  • Home pages
  • Organization pages

Clean up:

  • Clean-up

📸 Assets (if appropriate):

image

image

@0x5bfa 0x5bfa self-assigned this Sep 3, 2022
@0x5bfa 0x5bfa added type/feature-request This issue is a feature request. design labels Sep 3, 2022
@0x5bfa 0x5bfa added this to the FluentHub v0.5 milestone Sep 3, 2022
@0x5bfa 0x5bfa changed the title Improve error handling mothod Improve error handling mothod in user pages Sep 3, 2022
@Lamparter Lamparter self-requested a review September 3, 2022 17:46
@Lamparter Lamparter changed the title Improve error handling mothod in user pages Improve error handling Sep 3, 2022
@Lamparter Lamparter mentioned this pull request Sep 4, 2022
Copy link
Collaborator

@Lamparter Lamparter left a comment

Choose a reason for hiding this comment

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

@onein528 What on earth did you do b506654 for?

Repository owner deleted a comment from 0x5bfa Sep 4, 2022
@Lamparter
Copy link
Collaborator

@onein528 What on earth did you do b506654 for?

It breaks the search system.

@0x5bfa
Copy link
Owner Author

0x5bfa commented Sep 4, 2022

WHAT? No I didn't break any feature. Look at it carefully. I created some duplicated attribution by main fetching. So I deleted them.

@0x5bfa
Copy link
Owner Author

0x5bfa commented Sep 4, 2022

More information:

image

image

image

@Lamparter
Copy link
Collaborator

Ahh. Sorry.

@0x5bfa
Copy link
Owner Author

0x5bfa commented Sep 4, 2022

You can see nicer error page! Just try it out.

@Lamparter
Copy link
Collaborator

I ABSOLUTELY LOVE THIS
image
I will fix the spelling. Good work, @onein528! 😄

@Lamparter
Copy link
Collaborator

Lamparter commented Sep 4, 2022

BTW @onein528 Please use strings from now on.
I will localise this.

@0x5bfa
Copy link
Owner Author

0x5bfa commented Sep 4, 2022

OK! Thanks. I also will.

@Lamparter
Copy link
Collaborator

@onein528 I'd set a custom title bar element like in SignIn so the profile/home/explore/tabview etc. are hidden.

@0x5bfa
Copy link
Owner Author

0x5bfa commented Sep 4, 2022

Almost of all exceptions are not recoverable. But network disconnection exception titled 'An error occurred while sending the request' is recoverable and general exception error. I think the app should provide page as it is to restart immediately whenever network is back for user by clicking 'Try Again'. Yes design issue I know but this could be functional issue.

@Lamparter
Copy link
Collaborator

Almost of all exceptions are not recoverable. But network disconnectio exception titled 'An error occurred while sending the request' is reconverable and general exception error. I think the app should provide page as it is to restart immediately whenever network is back for user. Yes design issue I know but this could be functional issue.

But we still have a 'refresh' button. @onein528

@0x5bfa
Copy link
Owner Author

0x5bfa commented Sep 4, 2022

Let it be clear. clicking 'Try Again', that left-side navigation view will be back?

@Lamparter
Copy link
Collaborator

Let it be clear. clicking 'Try Again', that left-side navigation view will be back?

Yep. But only if the error is resolved after clicking 'Try Again' ofc. @onein528

@0x5bfa
Copy link
Owner Author

0x5bfa commented Sep 4, 2022

Good. I'll try to do that.

@0x5bfa
Copy link
Owner Author

0x5bfa commented Sep 5, 2022

Why did you reset label from high pri to medium pri?

@Lamparter
Copy link
Collaborator

Why did you reset label from high pri to medium pri?

Why so high priority? @onein528

@0x5bfa
Copy link
Owner Author

0x5bfa commented Sep 6, 2022

High: Must be
Med: Should be
Low: Could be

Committed.

@0x5bfa
Copy link
Owner Author

0x5bfa commented Sep 8, 2022

@DeveloperWOW64 can you see UserControls/Overview/RepositoryOverview.xaml and it's inside Stargizer count badge and it's converter? It is way better than previous converter as helper methods. What do you think?

@0x5bfa
Copy link
Owner Author

0x5bfa commented Sep 9, 2022

What's New

Updated pinned issues block UI

image

@0x5bfa 0x5bfa marked this pull request as ready for review September 11, 2022 03:18
Copy link
Collaborator

@ioapiset ioapiset left a comment

Choose a reason for hiding this comment

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

LGTM.

@github-actions github-actions bot added the pr-triage/ready-to-merge This PR has been approved by an FH member. label Sep 11, 2022
@0x5bfa 0x5bfa merged commit 0eba6ff into main Sep 11, 2022
@0x5bfa 0x5bfa deleted the error-handling branch September 11, 2022 03:36
@0x5bfa 0x5bfa removed type/feature-request This issue is a feature request. design labels Sep 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-triage/ready-to-merge This PR has been approved by an FH member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants