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

feat: Add generic error page and replace old ErrorView #877

Merged
merged 11 commits into from
Aug 3, 2023

Conversation

Ldoppea
Copy link
Member

@Ldoppea Ldoppea commented Jul 18, 2023

Note
This PR includes several refactor commits that move code and convert it to Typescript, feel free to read code commit by commit to remove noise

Current ErrorView implementation doesn't contains any design so the displayed error is very raw and is not user friendly

We want to improve this UI but instead of modifying the ErrorView we replace it by a new generic error page from ErrorScreen. This would improve code homogeneity

Also we now use the navigateToError function to analyse the error object and chose which error to display

Warning
Error message extracted from FetchErrors can still be technical or contain some JSON content. For now I chose to display as is but maybe we should display a generic message instead and show the error message only when clicking details button?

Result page

Note that the error message is from code, so it is not translated
image

Result page after clicking "voir les détails de l'erreur"

Note that the details are displayed using a js alert() so we cannot copy its content, but it should be ok for a first iteration
image

Copy link
Contributor

@acezard acezard left a comment

Choose a reason for hiding this comment

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

Overall, except the 5 comments, I have not much more to say. My main issue is all these makeHTML stuff. What do we lose by rendering them with native views? We have all the UI components needed now

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about that here, for me makeHtmlErrorPage is linked to view and not to domain (business logic)

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 moved it here: bf91bb6

I chose to merge the type definitions in the same file, are you ok with that?

src/app/domain/errors/models/ErrorPageGenerator.ts Outdated Show resolved Hide resolved
}: MakeErrorPageProps): string => {
const dimensions = getDimensions()
const navbarHeight = dimensions.navbarHeight
const statusBarHeight = dimensions.statusBarHeight

const theme = invertedTheme ? 'theme-inverted' : 'theme-light'
Copy link
Contributor

Choose a reason for hiding this comment

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

can't the string be passed directly as an enum in props? that way this view file doesn't handle this trinary operator logic

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 did the change here: a821d18

src/app/domain/errors/makeHtmlErrorPage.ts Outdated Show resolved Hide resolved
src/app/domain/errors/navigateToError.ts Outdated Show resolved Hide resolved
@Ldoppea
Copy link
Member Author

Ldoppea commented Jul 21, 2023

What do we lose by rendering them with native views? We have all the UI components needed now

The only thing we haven't done yet is to display SVG images in the native views.

@Ldoppea Ldoppea force-pushed the feat/generic_error_page branch 3 times, most recently from 7536879 to 91415ee Compare July 25, 2023 16:06
Base automatically changed from fix/not_onboarded_cozy to master July 26, 2023 16:47
In the next commit we will want to display an Error page using a light
background

This commits add the possibility to chose between light and inverted
themes

Default theme is inverted to keep compatibility with existing error
pages
In case of unexpected error we want to display a generic error page
that displays the error message and allow to display the error's
technical details if needed

This page is meant to replace the current ErrorView used in Login and
Onboarding screens. This replacement will be done in the following
commit
Current ErrorView implementation doesn't contains any design so the
displayed error is very raw and is not user friendly

We want to improve this UI but instead of modifying the ErrorView we
replace it by the new generic error page from ErrorScreen. This would
improve code homogeneity

Also we now use the `navigateToError` function to analyse the error
object and chose which error to display
Contact email is already displayed on the error body so we don't need
the footer to be displayed
@acezard acezard self-requested a review July 27, 2023 08:01
@Ldoppea Ldoppea merged commit b48d355 into master Aug 3, 2023
1 check passed
@Ldoppea Ldoppea deleted the feat/generic_error_page branch August 3, 2023 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants