Storybook and TS migration of ErrorPlaceholder component
#5294
Conversation
Stories include: * Default generic error * API error * Unknown error * Error without possibility to retry * Base Error atom Where applicable, the **Retry** button will not only trigger the appropriate action, but also simulate the loading state of the error component.
sunyatasattva
added
focus: components
rubikuserbot
requested review from
a team and
gigitux
and removed request for
a team
|
Size Change: -8 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
| /** | ||
| * Context in which the error was triggered. That will determine how the error is displayed to the user. | ||
| */ | ||
| type: 'api' | 'general'; |
There was a problem hiding this comment.
In these cases, I prefer to use an enum. This will allow for easy refactor in the future if there should be a need
There was a problem hiding this comment.
While I am also a fan of enums, I feel there are different use-cases which fits best each type, and one shouldn't default to enums, in general. Here is a quick rundown: https://blog.bam.tech/developer-news/should-you-use-enums-or-union-types-in-typescript and here also a nice SO opinion: https://stackoverflow.com/a/60041791/313115
In general, for example, enums are great for readability when their key is not equivalent to their value. Imagine something like:
{
keycode: 37 | 38 | 39 | 40
}
This doesn't give the reader much information on what's going on. Rather:
enum Keycode {
ARROW_LEFT = 37,
ARROW_UP = 38,
ARROW_RIGHT = 39,
ARROW_DOWN = 40
}
// ...
{
keycode: Keycode
}
The above is much more terse. In our case, the enum would be something like:
enum ErrorType {
API = 'api'
GENERAL = 'general'
}
I wouldn't say that's very helpful on the readability. I understand what you are saying regarding refactoring, but the SO answer above mentions iterating through a const, which I think it fits our case better. Something like:
const errorTypes = [ 'api', 'general' ] as const;
type ErrorTypes = typeof errorTypes[number];
Anyways, in this case, as we have only two types, I think this is slight premature optimization, so my opinion here is YAGNI.
But that's just my 2c, curious to hear what do you think. 🙏
There was a problem hiding this comment.
Thanks for sending me these resources!
About readability you can define an enum like this:
enum Error = {
API,
GENERAL
}
For me, the advantage of enums (and the killer feature too) is that we can easily rename for the entire project, instead of the string literal.
Anyways, in this case, as we have only two types, I think this is slight premature optimization, so my opinion here is YAGNI.
I agree, but potentially this component it's a "global" component, so it is better to think about it now that refactor later
There was a problem hiding this comment.
For me, the advantage of enums (and the killer feature too) is that we can easily rename for the entire project, instead of the string literal.
Mm, I see what you mean! However you could do the same renaming globally by just extracting the union type. There is no advantage in terms of refactoring between doing:
enum ErrorType = {
API,
GENERAL
}
and
type ErrorType = 'api' | 'general';
As long as you export both of those things and import them, refactoring is going to involve the exact amount of work. So I'm not sure if that's a significant advantage.
With that said, obviously, in this specific case it is actually a “hard-coded” string union type, so for refactoring reasons, definitely it might be better to extract it into its own type.
As a matter of fact, if you see the code changes, that's an approach I took elsewhere. Previously the code had:
error: PropTypes.shape( {
/**
* Human-readable error message to display.
*/
message: PropTypes.node,
/**
* Context in which the error was triggered. That will determine how the error is displayed to the user.
*/
type: PropTypes.oneOf( [ 'api', 'general' ] ),
} )
The above was already repeated twice, so I refactored it in its own type. Some go by the Rule of Three when refactoring, though I am a bit more extreme and I usually refactor earlier (some say to a fault). But I definitely agree with you there about long term thinking and also, again, I am a fan of using Enums! 🤓
…ce#5294) * Convert `ErrorPlaceholder` and `ErrorMessage` to TypeScript * Add stories for `ErrorPlaceholder` and `ErrorMessage` (woocommerce#5255) Stories include: * Default generic error * API error * Unknown error * Error without possibility to retry * Base Error atom Where applicable, the **Retry** button will not only trigger the appropriate action, but also simulate the loading state of the error component. * Update references to `ErrorMessage` component to leave the file extension out Fix woocommerce#5255 Refs woocommerce#5249
…ce#5294) * Convert `ErrorPlaceholder` and `ErrorMessage` to TypeScript * Add stories for `ErrorPlaceholder` and `ErrorMessage` (woocommerce#5255) Stories include: * Default generic error * API error * Unknown error * Error without possibility to retry * Base Error atom Where applicable, the **Retry** button will not only trigger the appropriate action, but also simulate the loading state of the error component. * Update references to `ErrorMessage` component to leave the file extension out Fix woocommerce#5255 Refs woocommerce#5249
This PR adds stories for the
ErrorPlaceholdercomponent and migrates it to TypeScript.Fixes #5255
Testing
Manual Testing
How to test the changes in this Pull Request:
Run
npm run storybookExplore the Errors stories. You should see five stories:
In all the variations with the retry button, you should be able to click it and it will simulate a loading event for 3.5s.
User Facing Testing
These are steps for user testing (where "user" is someone interacting with this change that is not editing any code).