feat: storybook v6 implementation along with global decorators #20
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -1 +1,2 @@ | |||
| EXPO_PUBLIC_APP_NAME= | |||
| EXPO_PUBLIC_STORYBOOK_ENABLED= | |||
There was a problem hiding this comment.
The pull request looks good, but I have a few suggestions:
- It would be helpful to have some documentation or comments explaining the purpose of
EXPO_PUBLIC_STORYBOOK_ENABLEDsince it's a new environment variable. - Consider adding a default value for
EXPO_PUBLIC_STORYBOOK_ENABLEDin case it's not set. This will prevent unexpected errors if the variable is referenced without being defined.
Overall, 👍 nice work on the pull request!
| @@ -8,6 +8,7 @@ module.exports = { | |||
| groups: [['^expo', '^react'], ['^@?\\w'], ['@/(.*)'], ['^[./]']], // sort order for imports | |||
| }, | |||
| ], | |||
| 'import/order': ['off'], | |||
| 'import/newline-after-import': ['error', { count: 1 }], // new line after all the imports | |||
| 'padding-line-between-statements': [ | |||
| // blank line between statements | |||
There was a problem hiding this comment.
The code changes look good. 👍🏼 I see that you're turning off the import/order rule to allow custom sorting order for imports.
Just a minor suggestion, if I may: consider adding comments or documentation to explain why import/order is being turned off and which custom sorting order is being used (if any).
Other than that, everything seems fine! 😊
|
|
||
| const StorybookUIRoot = getStorybookUI({}); | ||
|
|
||
| export default StorybookUIRoot; |
There was a problem hiding this comment.
👍 The code looks good to me. It imports the 'getStorybookUI' function from '@storybook/react-native', which is used to create the root component for the Storybook UI. It then calls that function with an empty configuration object to create the 'StorybookUIRoot' component and exports it.
A potential improvement could be to add comments/documentation to explain what each line of code does and any dependencies or requirements to run the code.
| '@storybook/addon-ondevice-controls', | ||
| '@storybook/addon-ondevice-actions', | ||
| ], | ||
| }; |
There was a problem hiding this comment.
👍 The code looks good.
One suggestion for improvement could be to add comments explaining the purpose of the different parts of the configuration. This can make it easier for other developers to understand and modify the code in the future. For example:
// Configure storybook to look for story files in both .mdx and .js/.jsx/.ts/.tsx formats
stories: ['../src/**/*.stories.mdx', '../src/**/*.stories.@(js|jsx|ts|tsx)'],
// Add two addons to Storybook - ondevice-controls and ondevice-actions
addons: [
'@storybook/addon-ondevice-controls',
'@storybook/addon-ondevice-actions',
],
Adding such comments can make it easier to maintain the codebase and onboard new developers.
| <Story /> | ||
| </View> | ||
| ), | ||
| ]; |
There was a problem hiding this comment.
Overall, the code looks good! Here are a few suggestions:
- Consider commenting on what each section of the code does, especially for the decorators array.
- Use more descriptive names for the components and matchers properties in the controls matcher to enhance readability. For example, instead of 'color', you could use 'backgroundColor' or 'textColor'.
- Consider adding some error checking to prevent undefined values when accessing certain properties in the code.
👍 Overall, good job!
| }; | ||
| }; | ||
|
|
||
| configure(getStories, module, false); |
There was a problem hiding this comment.
The code seems fine. However, here are some minor suggestions that can be considered:
- Add comments to help understand the purpose of each section of the code.
- Use consistent quotation marks (
''or"") throughout the code. - Use camelCase or snake_case instead of kebab-case in naming variables and methods.
- Consider using a try-catch block with specific error handling instead of an empty catch block.
- Try to avoid unnecessary console warnings for debugging purposes.
| justifyContent: 'center', | ||
| }, | ||
| }); | ||
| export default EntryPoint; |
There was a problem hiding this comment.
The code looks good. The unnecessary imports have been removed, and the component for the entry point of the application has been updated to HomePage. However, it would be helpful to verify that the env variable is no longer being used in other parts of the application before removing it completely to avoid any unintended effects. Overall, 👍 great job on this pull request!
|
|
||
| ### Storybook | ||
|
|
||
| Configured with [storybook-react-native](https://github.com/storybookjs/react-native) to isolate and test UI components. Configured with global decorators and easy to write storybook under each components. |
There was a problem hiding this comment.
The pull request looks good! The changes to the task list seem to be appropriate and you have provided a brief description of the Storybook configuration. The only suggestion would be to improve formatting consistency, using consistent punctuation and capitalization throughout.
| web: { | ||
| favicon: './assets/favicon.png', | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The code looks good and concise. However, here are some suggestions:
- Consider adding comments to the code to explain what each property does
- Use consistent quotation marks, either single or double quotes, throughout the code
- When providing feedback in Pull Requests, it's a good practice to start a new line for each comment and use emojis such as ✔️ (check mark) or ❌(cross mark) to clarify whether changes are needed or not.
| }, | ||
| }, | ||
| ], | ||
| ], | ||
| }; | ||
| }; |
There was a problem hiding this comment.
👍🏼 Overall, the code looks good and well-organized.
👉🏼 Here are a few suggestions to consider:
- It's a good practice to add comments explaining what each part of the code does, especially for more complex functions like this one.
- If you plan on adding more plugins in the future, you could consider extracting them into their own array inside the
pluginskey. This would make it easier to manage and update them. - You might want to add error handling for cases where the
apiargument isundefinedor not an object.
| }); | ||
|
|
||
| config.resolver.resolverMainFields.unshift('sbmodern'); | ||
| module.exports = config; |
There was a problem hiding this comment.
The code looks good overall. Here are a few suggestions to make it even better:
- It's good practice to have consistent indentation. You can use either 2 spaces or 4 spaces, but it's important to be consistent throughout the file.
- The commented-out code on line 1 (
/* eslint-disable no-undef */) should be removed or uncommented if it's necessary to disable ESLint for this file. - Add a descriptive comment to explain what the code on lines 5-10 is doing.
- Consider adding a space after the commas in the object literal on lines 7-9 for consistency.
👍 Good job on using JSDoc comments to document the type of the config object.
💡 As a suggestion, you could use more descriptive variable and function names to make the code easier to understand and maintain in the future.
| "react-native-bundle-visualizer": "^3.1.3", | ||
| "react-native-safe-area-context": "4.6.3", | ||
| "skott": "^0.28.1", | ||
| "typescript": "^5.1.3" | ||
| }, |
There was a problem hiding this comment.
Here are my suggestions:
- Good job adding clarity to the script names, but it may be helpful to add brief descriptions or comments explaining what some of the less common scripts do. 📜
- Consider using a consistent versioning strategy across dependencies to make updates and maintenance easier. 🔄
- It may be beneficial to update the version of "react-native" to the latest stable version for security and performance improvements. 🆙
- Adding "@babel/preset-env" as a devDependency can improve cross-browser compatibility. 🌐
- You've added Storybook dependencies, which is great, but consider adding an explanation to your README.md file about how to run Storybook. 📝
| @@ -1,2 +1,2 @@ | |||
| // Do not add any other lines of code to this file! | |||
| import "@total-typescript/ts-reset"; | |||
| import '@total-typescript/ts-reset'; | |||
There was a problem hiding this comment.
The pull request seems to be a minor change in code formatting. The only change made was to switch the quotes used around the imported module name from double quotes to single quotes.
One suggestion could be to ensure that the code style throughout the project is consistent with either double or single quotes, to avoid confusion and improve readability.
Overall, the change looks good and can be merged without issues. 👍
|
|
||
| storiesOf('Button', module) | ||
| .add('Default', () => <Primary />) | ||
| .add('Transparent', () => <Secondary />); |
There was a problem hiding this comment.
👍 The code looks good!
One suggestion I have is to add some comments to explain what each part of the code does. This will make it easier for anyone who reads your code (including future you!) to understand its purpose.
You may also want to consider adding some more test cases to ensure that the component behaves as expected in different scenarios. Overall, good job on this code snippet!
| borderWidth: 1, | ||
| borderColor: 'purple', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The code looks good overall. Here are a few suggestions for improvement:
- Consider adding some comments to the code to make it easier for other developers to understand what is happening.
- It might be helpful to use an enum instead of a string type for the
typeproperty. This would ensure that only valid values are used and avoid typos in the future. - Instead of hardcoding the styling values in the component, you could consider allowing them to be passed in as props. This would allow greater flexibility in how the Button component is used.
- Consider using more descriptive variable names to make the code easier to read and understand.
- Consider using emojis in your commit messages and pull request descriptions to help make the communication more engaging and effective.
| EXPO_PUBLIC_STORYBOOK_ENABLED: z.union([ | ||
| z.literal('true'), | ||
| z.literal('false'), | ||
| ]), | ||
| }, | ||
| /** | ||
| * What object holds the environment variables at runtime. |
There was a problem hiding this comment.
Great job adding support for a new environment variable! 👍
Here are a few suggestions to improve the code further:
- It might be better to rename the
EXPO_PUBLIC_STORYBOOK_ENABLEDvariable to something more generic, likeENABLE_STORYBOOK, so it's more flexible and can be used in different contexts. - Consider using a boolean type instead of a union of string literals for
EXPO_PUBLIC_STORYBOOK_ENABLED. This way, it's easier to work with the variable in code and avoid any potential issues that could arise from using a string comparison. You could usez.boolean()for this. - Add comments explaining what the new variable is used for and how to set it if necessary.
| EntryPoint = StorybookUIRoot; | ||
| } | ||
|
|
||
| export default EntryPoint; |
There was a problem hiding this comment.
Overall, the code looks good. Here are a few suggestions for improvement:
-
Add comments to explain what the code is doing. This can help anyone else who reads the code understand it more easily.
-
Use consistent formatting with curly braces. Some parts of the code use the same line for the opening brace, while others put it on a new line. It's best to choose one and stick with it for consistency.
-
Consider adding error checking for
envvariables. If there's an error retrieving a value, it could cause unexpected behavior in the app. -
Use emojis sparingly and only when they add clarity or emphasis to your feedback.
Here's an updated version of the code with these suggestions implemented:
import { StatusBar } from 'expo-status-bar';
import { StyleSheet, Text, View } from 'react-native';
import StorybookUIRoot from '@storybook'; // add a comment explaining what this is
import { env } from '@/env';
function HomePage() {
return (
<View style={styles.container}>
<StatusBar style="auto" />
<Text>APP NAME: {env.EXPO_PUBLIC_APP_NAME}</Text>
<Text>Open up App.tsx to start working on your app!</Text>
</View>
);
}
const styles = StyleSheet.create({
container: {
flex: 1,
backgroundColor: '#fff',
alignItems: 'center',
justifyContent: 'center',
},
});
let EntryPoint = HomePage;
// add error checking for env variables
if (env.EXPO_PUBLIC_STORYBOOK_ENABLED && env.EXPO_PUBLIC_STORYBOOK_ENABLED === 'true') {
EntryPoint = StorybookUIRoot;
}
export default EntryPoint;
| "./src/*" | ||
| ], | ||
| "@storybook": [ | ||
| "./.storybook/index.js" | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
👍 Overall, the changes in this pull request seem reasonable. However, here are a few suggestions that could improve the code:
- It's usually a good practice to specify an explicit target in the
compilerOptionssection. For instance, you could add"target": "esnext"to enable modern JavaScript features. - The
baseUrloption is set to "." but it's not entirely clear what this means without additional context. You might consider adding a comment describing the intended behavior. - Similarly, the
@/*path appears to be a custom alias, so it would be helpful to document its usage for other maintainers who may be unfamiliar with the project. - Lastly, it seems like the
@storybookpath is specific to Storybook-related files. Consider renaming it to something more descriptive to avoid potential confusion in the future.
Other than these minor points, the proposed changes look good to me! Let me know if you have any questions.
| }, | ||
| }, | ||
| ], | ||
| ], | ||
| }; | ||
| }; |
There was a problem hiding this comment.
👍 Overall, the pull request looks good. Here are a few suggestions to improve the code:
- It is good practice to have space around the arrow function parameter
function (api)should befunction(api). - Consider adding comments to clarify what each plugin does and why it's needed.
- Make sure that the aliases defined in plugins match with the actual file structure in the project.
- You can also consider adding more plugins if necessary for your project.
Overall, well done!
|
|
||
| export default EntryPoint; | ||
|
|
||
| registerRootComponent(EntryPoint); |
There was a problem hiding this comment.
👍 This code looks good to me!
It's a basic Expo entry point file where you import the registerRootComponent from expo and your root component, which is EntryPoint. Then you export it as default and call registerRootComponent(EntryPoint) to register it as your app's root component.
There are no obvious errors or issues with the code. However, if you wanted to improve it, you could consider adding comments to explain what each line does or why you're doing certain things. This can make the code more readable and easier to maintain in the future.
| "react-native-bundle-visualizer": "^3.1.3", | ||
| "react-native-safe-area-context": "4.6.3", | ||
| "skott": "^0.28.1", | ||
| "typescript": "^5.1.3" | ||
| }, |
There was a problem hiding this comment.
Overall, the code changes seem fine. Here are a few suggestions:
- It's good that the
iosscript now includes the-cflag to clear the Metro bundler cache for iOS. - Consider adding some comments explaining what the new scripts do (
storybook-generateandstorybook-watch). - Consider updating
react-nativeto the latest version (currently 0.66.4) to ensure you have the latest features, bug fixes, and security updates. - Is there a specific reason for using
"react-dom": "^18.2.0"since it's typically used for web development in conjunction with React? If not, you can probably remove this dependency. - For future pull requests, consider adding a more detailed description of the changes made and the reasoning behind them to help reviewers understand the context better. 😊
| "./src/*" | ||
| ], | ||
| "@storybookui": [ | ||
| "./.storybook/index.js" | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
👍 The code looks good!
The only suggestion I would make is to add comments to explain the purpose of "include": ["src"] and "baseUrl": ".". This would help other developers better understand the configuration file.
Other than that, great job!
| @@ -8,6 +8,7 @@ module.exports = { | |||
| groups: [['^expo', '^react'], ['^@?\\w'], ['@/(.*)'], ['^[./]']], // sort order for imports | |||
| }, | |||
| ], | |||
| 'import/order': ['off'], // keepingdefault rule off and replaced with simple import sort | |||
| 'import/newline-after-import': ['error', { count: 1 }], // new line after all the imports | |||
| 'padding-line-between-statements': [ | |||
| // blank line between statements | |||
There was a problem hiding this comment.
Based on the provided code, it looks like you have turned off the default 'import/order' rule and added a 'padding-line-between-statements' configuration. However, it would be good to add comments explaining why these changes were made and provide more context.
👍 Consider adding comments to explain the purpose of turning off the default 'import/order' rule and enabling 'padding-line-between-statements'.
👎 It's generally better to avoid one-word comments such as "off" and instead provide a brief explanation of why the rule is being turned off.
Overall, the code changes seem fine, but providing more informative comments can make it easier for other developers to understand and maintain the code in the future.
No description provided.