twenty icon indicating copy to clipboard operation
twenty copied to clipboard

fix: settings error layout

Open harshrajeevsingh opened this issue 1 year ago • 15 comments

Fixes: #7460

Screenshot from 2024-10-14 15-27-52

Changes & Why

Since all the settings pages lie in the Outlet of DefaultLayout, there was no way to handle it apart from creating a separate errorFallback for the settings route. So, I created a settingsErrorFallback component that uses the same styling of settings pages. Created ErrorBoundaryWrapper that checks if its settings route then show SettingsErrorFallback else show GenericErrorFallback. Now, for the breadcrumb part. I found that all the settings pages use hardcoded title. So, I created generateBreadcrumbLinks function that will provide different title and links based on how it's respective settings page has them. If this approach looks fine, I will add the other remaining titles and links to the generateBreadcrumbLinks function and move that whole function to its separate file. And will fix linting errors.

If there is any different approach to handle it, lemme know. I'm happy to implement it.

harshrajeevsingh avatar Oct 14 '24 10:10 harshrajeevsingh

@charlesBochet I need your review on this.

harshrajeevsingh avatar Oct 14 '24 10:10 harshrajeevsingh

@Bonapara Should we implement this not only in settings but everywhere in the app ?

lucasbordeau avatar Oct 18 '24 17:10 lucasbordeau

@Bonapara Should we implement this not only in settings but everywhere in the app ?

https://github.com/user-attachments/assets/c2973008-6f2e-4881-a3de-b8f9c2d10504

Don't we already do this?

Bonapara avatar Oct 18 '24 17:10 Bonapara

Fixes: #7460

Screenshot from 2024-10-14 15-27-52

Thanks for contributing @harshrajeevsingh! Can you remove the Error Occured page title?

Bonapara avatar Oct 18 '24 17:10 Bonapara

Fixes: #7460 Screenshot from 2024-10-14 15-27-52

Thanks for contributing @harshrajeevsingh! Can you remove the Error Occured page title?

Yeah! I will remove it. @lucasbordeau any suggestion from your side? Also in which folder should I move the generateBreadcrumbLinks function after adding the other page's missing links & title?

harshrajeevsingh avatar Oct 18 '24 19:10 harshrajeevsingh

@Bonapara Should we implement this not only in settings but everywhere in the app ?

CleanShot.2024-10-18.at.19.02.25.mp4 Don't we already do this?

@Bonapara I don't think so

Screenshot from 2024-10-19 19-53-19

harshrajeevsingh avatar Oct 19 '24 14:10 harshrajeevsingh

Oh you're right @harshrajeevsingh, thanks.

image

All the designs are here:

https://www.figma.com/design/xt8O9mFeLl46C5InWwoMrN/Twenty?node-id=19439-92929&node-type=frame&t=n39yTGMmRAB8gzNq-11

Bonapara avatar Oct 21 '24 07:10 Bonapara

@harshrajeevsingh modules/error-handlers seems like the right place ?

lucasbordeau avatar Oct 21 '24 10:10 lucasbordeau

@Bonapara Do we need that view bar, too? I think it's better to throw errors for that whole container. If any error occurs in viewBar, too, it will show the default GenericErrorFallback. @lucasbordeau wdyt?

Screenshot from 2024-10-22 00-43-45

harshrajeevsingh avatar Oct 22 '24 06:10 harshrajeevsingh

Hi @harshrajeevsingh, no we do not need the view bar, especially if it's a page error and not a table error

Bonapara avatar Oct 22 '24 08:10 Bonapara

@lucasbordeau what should be passed in header of both RecordIndexError & RecordShowError fallback and how? For now, I just gave a temporary title.

Screenshot from 2024-10-23 00-45-43

harshrajeevsingh avatar Oct 22 '24 19:10 harshrajeevsingh

/award 150

charlesBochet avatar Oct 31 '24 12:10 charlesBochet

Awarding harshrajeevsingh: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/harshrajeevsingh

oss-gg[bot] avatar Oct 31 '24 12:10 oss-gg[bot]

@harshrajeevsingh Let's try to keep the breadcrumb as it was before the error occurs, and then make sure the new white background is on every error page. We don't need to spend too much time on this.

lucasbordeau avatar Oct 31 '24 17:10 lucasbordeau

@lucasbordeau Sorry, I didn’t quite understand. Are you asking me to remove the whole breadcrumbs part and just use a white container for every page with no headings or titles?

Like this?

Screenshot from 2024-10-23 00-10-06

harshrajeevsingh avatar Nov 01 '24 13:11 harshrajeevsingh

Yes please, also remove the breadcrumb computing part in your code, we'll see that later, thank you.

lucasbordeau avatar Nov 04 '24 13:11 lucasbordeau