add Error component #484

Ulisseus · 2021-02-02T15:05:38Z

This PR adds new Error component. Part of #467.

vercel · 2021-02-02T15:05:42Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/c0d3/c0d3-app/27rl0luxw
✅ Preview: https://c0d3-app-git-fork-ulisseus-error-main.c0d3.vercel.app

codecov · 2021-02-02T15:08:46Z

Codecov Report

Merging #484 (38e352b) into master (c17afea) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #484   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           94        95    +1     
  Lines         1284      1287    +3     
  Branches       268       268           
=========================================
+ Hits          1284      1287    +3

Impacted Files	Coverage Δ
components/Error.tsx	`100.00% <100.00%> (ø)`

anthonykhoa · 2021-02-02T23:40:57Z

components/Error.tsx

+  src: string
+  message?: string
+}
+const Error: React.FC<ErrorProps> = ({ title, message, src }) => {


Are the images bound to specific messages and or titles? For example, every time message equals No data, do we want to display the 500 error image with a Internal server error title?

Similarly, when the message equals 404 error, do we want to always display the 404 error image with a Page not found title? If we have images that are only used for specific error messages or titles, then I don't think we should pass in src as a prop. Rather there can be a single type prop that determines the title, image src, and message to use.

If it is the case that the image src is not bound to any error message or title, then I think it would be better to have src have only a limited number of options(not found, 404) as a valid input. And instead of src it should be renamed something like type. A table can be made to match the corresponding image src depending on value of type passed in. This would make the component easier for others to use in the future, since they won't have to determine the correct paths to use as a value for src.

Something like this:

enum ImageType { NOT_FOUND = '500.png', 404 = '404.png' }

Titles and background images are tied together. Page not found is shown with a confused man, Internal server error results in generic server image, so it makes sense to merge them in a new type somehow. I am not sure if enum is the best way to do so though, why not a simple union?

type ErrorProps = { type: '404' | '500' message?: string } const Error: React.FC<ErrorProps> = ({ type, message }) => { ... <Text component="div" size="xl" bold={true}> {type === '404' ? 'Page not found' : 'Internal server error'} </Text ....

This way you don't need to export enum every time you need to use Error component.

Yep, unions look like they work too! I only used enums as an example, which is why I said something like this. I apologize for the misunderstanding, I should have been more clear in my language and emphasize to make the component easier for others to use in the future as the main goal

OK, I changed it to union type. If we need more error pages we could extend this union and change ternary operator for a switch.

anthonykhoa · 2021-02-05T20:23:29Z

components/Error.tsx

+          </div>
+          <div className="col-sm-9">
+            <img
+              src={type === '404' ? '/404.png' : '/500.png'}


Based on the choices for type, the image src paths shouldn't have to be hardcoded.

src={`/${type}.png`}

anthonykhoa · 2021-02-05T20:36:38Z

components/Error.tsx

+          <div className="d-flex col-sm-3 align-items-center justify-content-center">
+            <div className="text-center mt-3">
+              <Text component="div" size="xl" bold={true}>
+                {type === '404' ? 'Page not found' : 'Internal server error'}


I see a high potential for someone else updating this component in the future, to add another type of error(for example, admin pages can have an unauthorized error for non-admins).

Therefore, I think this piece of logic can be refactored so that it is easier for others to add another error type in the future.

Right now, the next person to add onto this file may experience these difficulties:

Having to visually filter through html code to determine where to edit in the file

Layout -> div -> div -> div -> div -> Text: Have to dig all the way here to find the logic

Figuring out that 500 is the error number associated with Internal server error

With a ternary operator, it's not explicitly clear that Internal server error is the intended title for error 500. A person would have to analyze the entire component as a whole, and then make the connection of Oh, there's only one other error type, and it is error 500. So I guess Internal server error is the title for 500.

Figuring out the best way to add a third option

Continuing to use a ternary operator to support a third error type makes the code unnecessarily complex and more unreadable. The next person to add another type to this file would have to go through the process of deciding how to best support a third error type.

Your initial enum suggestion would be the best approach here in my opinion. With a enum we can directly map an error code to it's associated image, not even needing a ternary operator or switch. And it's also easy to modify in the future by adding new error status codes.

For example:

// status code enum enum StatusCode { NOT_FOUND = '404', INTERNAL_SERVER_ERROR = '500' } // default messages map const errorMessages: ReadonlyMap<StatusCode, string> = new Map([ [StatusCode.NOT_FOUND, 'Page not found'], [StatusCode.INTERNAL_SERVER_ERROR, 'No data'] ]) // passing as a prop to the component <Error type={StatusCode.NOT_FOUND} ... /> // using on the img src <img src={`/${type}.png`} /> // get the default message <Text> {errorMessages.get(type)} </Text>

OK, I changed it to enum. It's easier to add new pages this way, on the other hand, using it would be easier with unions IMO since you don't need to export (and figure out extra type). But it's such a minor thing so I'm fine with either.

I think using the map object makes things more complex than they have to be. Map is meant to be used when we have to set memory addresses(such as objects) as key values, but all the status codes values are just strings.

To be more simple, we can just use a regular object:

const errorTitles: Readonly<{ [key in StatusCode]: string }> = { [StatusCode.NOT_FOUND]: 'Page not found', [StatusCode.INTERNAL_SERVER_ERROR]: 'Internal server error' }

Also, since there is already a message prop that is passed into the Error component, I find it confusing that there is another errorMessages table. I think a more appropriate variable name is errorTitles, or something that doesn't have message in it

Makes sense, I thought that map was faster than object for accessing values (turns out it is faster but for inserting/removing key-values pairs). It's an object now.

anthonykhoa · 2021-02-08T00:47:36Z

components/Error.tsx

  message?: string
 }
-const Error: React.FC<ErrorProps> = ({ type, message }) => {
+const Error: React.FC<ErrorProps> = ({ code, message }) => {
+  const errorMessages: ReadonlyMap<StatusCode, string> = new Map([


I think this hash table should be on the global scope. The table has constant values that do not depend on the value of props passed into the component. As such, there is no need to generate another errorMessages table every time this component is used.

Mapping from status codes is out of component scope and it's renamed to errorTitle.

…o Error/main

anthonykhoa · 2021-02-08T19:15:13Z

stories/components/Error.stories.tsx

+}
+
+export const Internal: React.FC = () => {
+  const mocks = [


I think this file can be refactored so that there is less duplicated code. The mocks are exactly the same value in each storybook entry(Internal, NotFound).

anthonykhoa · 2021-02-09T00:21:04Z

components/Error.tsx

+import NavLink from './NavLink'
+import { Text } from './theme/Text'
+export enum StatusCode {
+  NOT_FOUND = '404',


Why are the status code numbers strings? I think it would make more sense to use numbers as numbers, rather than as strings.

Additionally, this removes the need to have a toStatusCode function that you have in your PR#485

anthonykhoa · 2021-02-09T02:33:54Z

components/Error.tsx

+              </NavLink>
+            </div>
+          </div>
+          <div className="col-sm-9">


This div is unneeded, you can just add the classname(col-sm-9) to the image's classes

anthonykhoa · 2021-02-09T05:33:06Z

The two images should be svg's because:

svg's scale easily without losing quality, while png's lose quality upon scaling.
The rest of the images in our public folder are svg's, so for the sake of codebase consistency these two images should also be svg's as well.

Ulisseus · 2021-02-09T12:05:58Z

The two images should be svg's because:

1. `svg`'s scale easily without losing quality, while `png`'s lose quality upon scaling.

2. The rest of the images in our `public` folder are `svg`'s, so for the sake of codebase consistency these two images should also be `svg`'s as well.

Resaving bitmap as svg won't help much. I don't have original source so it will look look as bad as upscaled png. Should I change it just for the sake of consistency? It would be misleading IMO, it's still a bitmap just in different format. I also added next/image component which should help with image optimization.

ggwadera · 2021-02-09T14:25:48Z

Resaving bitmap as svg won't help much. I don't have original source so it will look look as bad as upscaled png. Should I change it just for the sake of consistency? It would be misleading IMO, it's still a bitmap just in different format. I also added next/image component which should help with image optimization.

You're right that just converting the png won't help. But even the Next image component won't help with scaling issues. Could you search for some another illustrations then? You can find some free resources here.

Ulisseus · 2021-02-09T16:37:13Z

OK, I changed images to svg, see new storybook. I liked the old one better but I couldn't find its source.

ggwadera · 2021-02-09T17:54:40Z

Will you be writing unit tests for this component as well?

Ulisseus · 2021-02-09T18:11:52Z

Will you be writing unit tests for this component as well?

But what is there to test? Error component has no internal logic, storybook snapshots should cover everything.

anthonykhoa · 2021-02-09T21:29:04Z

public/500.svg

@@ -0,0 +1,553 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>


We shouldn't put all the error images into the root of the public folder. To be more organized, they should be placed into a folder of their own

add Error component

efd2b47

vercel bot deployed to Preview February 2, 2021 15:05 View deployment

Ulisseus mentioned this pull request Feb 2, 2021

updates #487 - update pages routes #485

Merged

anthonykhoa reviewed Feb 2, 2021

View reviewed changes

change Error types

08e866f

vercel bot deployed to Preview February 4, 2021 09:35 View deployment

ggwadera approved these changes Feb 4, 2021

View reviewed changes

anthonykhoa reviewed Feb 5, 2021

View reviewed changes

change Error props to enum

f497d8e

vercel bot deployed to Preview February 7, 2021 13:15 View deployment

anthonykhoa reviewed Feb 8, 2021

View reviewed changes

Ulisseus added 2 commits February 8, 2021 11:51

Merge branch 'master' of https://github.com/garageScript/c0d3-app int…

28dd6e7

…o Error/main

rename errorMessages

b2b11e8

vercel bot deployed to Preview February 8, 2021 09:18 View deployment

anthonykhoa reviewed Feb 8, 2021

View reviewed changes

anthonykhoa reviewed Feb 9, 2021

View reviewed changes

add next/image, change enum type

2d70f9b

vercel bot deployed to Preview February 9, 2021 12:10 View deployment

update storybook snapshots

5d56dc4

vercel bot deployed to Preview February 9, 2021 12:16 View deployment

change images to svg

a1f6060

vercel bot deployed to Preview February 9, 2021 16:30 View deployment

anthonykhoa reviewed Feb 9, 2021

View reviewed changes

songz approved these changes Feb 10, 2021

View reviewed changes

hwong0305 approved these changes Feb 10, 2021

View reviewed changes

ggwadera merged commit 7e5e0fb into garageScript:master Feb 10, 2021

ggwadera mentioned this pull request Feb 11, 2021

[Feature] Better error screens #467

Closed

Ulisseus deleted the Error/main branch March 5, 2021 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Error component #484

add Error component #484

Ulisseus commented Feb 2, 2021 •

edited

Loading

vercel bot commented Feb 2, 2021 •

edited

Loading

codecov bot commented Feb 2, 2021 •

edited

Loading

anthonykhoa Feb 2, 2021 •

edited

Loading

Ulisseus Feb 3, 2021 •

edited

Loading

anthonykhoa Feb 3, 2021 •

edited

Loading

Ulisseus Feb 4, 2021

anthonykhoa Feb 5, 2021

anthonykhoa Feb 5, 2021 •

edited

Loading

ggwadera Feb 5, 2021 •

edited

Loading

Ulisseus Feb 7, 2021

anthonykhoa Feb 8, 2021 •

edited

Loading

Ulisseus Feb 8, 2021

anthonykhoa Feb 8, 2021 •

edited

Loading

Ulisseus Feb 8, 2021

anthonykhoa Feb 8, 2021 •

edited

Loading

anthonykhoa Feb 9, 2021

anthonykhoa Feb 9, 2021

anthonykhoa commented Feb 9, 2021 •

edited

Loading

Ulisseus commented Feb 9, 2021 •

edited

Loading

ggwadera commented Feb 9, 2021

Ulisseus commented Feb 9, 2021 •

edited

Loading

ggwadera commented Feb 9, 2021 •

edited

Loading

Ulisseus commented Feb 9, 2021 •

edited

Loading

anthonykhoa Feb 9, 2021 •

edited

Loading

		@@ -0,0 +1,553 @@
		<?xml version="1.0" encoding="UTF-8" standalone="no"?>

add Error component #484

add Error component #484

Conversation

Ulisseus commented Feb 2, 2021 • edited Loading

vercel bot commented Feb 2, 2021 • edited Loading

codecov bot commented Feb 2, 2021 • edited Loading

Codecov Report

anthonykhoa Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

Ulisseus Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

anthonykhoa Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

Ulisseus Feb 4, 2021

Choose a reason for hiding this comment

anthonykhoa Feb 5, 2021

Choose a reason for hiding this comment

anthonykhoa Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

ggwadera Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

Ulisseus Feb 7, 2021

Choose a reason for hiding this comment

anthonykhoa Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

Ulisseus Feb 8, 2021

Choose a reason for hiding this comment

anthonykhoa Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

Ulisseus Feb 8, 2021

Choose a reason for hiding this comment

anthonykhoa Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

anthonykhoa Feb 9, 2021

Choose a reason for hiding this comment

anthonykhoa Feb 9, 2021

Choose a reason for hiding this comment

anthonykhoa commented Feb 9, 2021 • edited Loading

Ulisseus commented Feb 9, 2021 • edited Loading

ggwadera commented Feb 9, 2021

Ulisseus commented Feb 9, 2021 • edited Loading

ggwadera commented Feb 9, 2021 • edited Loading

Ulisseus commented Feb 9, 2021 • edited Loading

anthonykhoa Feb 9, 2021 • edited Loading

Choose a reason for hiding this comment

Ulisseus commented Feb 2, 2021 •

edited

Loading

vercel bot commented Feb 2, 2021 •

edited

Loading

codecov bot commented Feb 2, 2021 •

edited

Loading

anthonykhoa Feb 2, 2021 •

edited

Loading

Ulisseus Feb 3, 2021 •

edited

Loading

anthonykhoa Feb 3, 2021 •

edited

Loading

anthonykhoa Feb 5, 2021 •

edited

Loading

ggwadera Feb 5, 2021 •

edited

Loading

anthonykhoa Feb 8, 2021 •

edited

Loading

anthonykhoa Feb 8, 2021 •

edited

Loading

anthonykhoa Feb 8, 2021 •

edited

Loading

anthonykhoa commented Feb 9, 2021 •

edited

Loading

Ulisseus commented Feb 9, 2021 •

edited

Loading

Ulisseus commented Feb 9, 2021 •

edited

Loading

ggwadera commented Feb 9, 2021 •

edited

Loading

Ulisseus commented Feb 9, 2021 •

edited

Loading

anthonykhoa Feb 9, 2021 •

edited

Loading