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

chore: imporove dataTable when embed #402

Merged
merged 1 commit into from
Jul 5, 2024
Merged

chore: imporove dataTable when embed #402

merged 1 commit into from
Jul 5, 2024

Conversation

islxyqwe
Copy link
Member

@islxyqwe islxyqwe commented Jul 5, 2024

Imporove the apperence when datatable is embed into a small container

Copy link

vercel bot commented Jul 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
graphic-walker ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 5, 2024 4:47am

Copy link
Contributor

github-actions bot commented Jul 5, 2024

Risk Level 2 - /home/runner/work/graphic-walker/graphic-walker/packages/graphic-walker/src/Table.tsx

The code changes in this pull request are relatively safe. However, there are a few areas that could be improved for better readability and maintainability:

  1. Use of magic numbers: The number 501 is used directly in the reportError function. It would be better to define it as a constant at the top of the file with a descriptive name. This makes the code easier to understand and maintain.
const ERROR_CODE = 501;
// ...
reportError(parseErrorMessage(err), ERROR_CODE)
  1. Error handling: The error message 'Something went wrong' is quite generic. It would be better to provide a more specific error message or even better, a way to recover from the error.

  2. Code comments: There are some comments in the code that indicate future work (// @TODO remove deprecated props). It's good practice to address these TODOs as soon as possible to prevent technical debt from accumulating.

  3. Props destructuring: In the TableAppWithContext function, the props object is destructured twice. It would be cleaner to destructure it once at the start of the function.

const { dark, dataSource, computation, onMetaChange, fieldKeyGuard, keepAlive, storeRef, defaultConfig, appearance = dark, data = dataSource, fields = rawFields ?? [], ...rest } = props;

Risk Level 2 - /home/runner/work/graphic-walker/graphic-walker/packages/graphic-walker/src/renderer/pureRenderer.tsx

The code seems to be well written and follows good practices. However, there are a few areas that could be improved for better readability and maintainability:

  1. Deprecation Warnings: There are several deprecated properties in the IPureRendererProps type. It would be helpful to add comments explaining why these properties are deprecated and what should be used instead.

  2. Use of unstable_batchedUpdates: This function is experimental and its API might change. Be aware of potential future changes that could break this code.

  3. Complexity of useMemo and useEffect hooks: The logic inside these hooks is quite complex and could be difficult to understand for someone unfamiliar with the code. Consider breaking down the logic into smaller, more manageable functions.

  4. Consistent Naming: The variable waiting could be renamed to isLoading to better reflect its purpose and to be consistent with common naming conventions.


Risk Level 2 - /home/runner/work/graphic-walker/graphic-walker/packages/playground/src/examples/pages/inModal.stories.tsx

The code is generally well written, but there are a few areas that could be improved:

  1. Inline Styles: There are several inline styles used in the code. Consider moving these to a CSS file or using a CSS-in-JS solution for better maintainability and reusability.

  2. Magic Numbers: The code uses magic numbers (e.g., blur(10px)). Consider defining these as constants at the top of the file or in a separate constants file.

  3. Event Handling: The onClick handler for the modal background stops propagation and prevents the default event. This could potentially interfere with other event handlers. Ensure this is the intended behavior.


🔮💡📚


Powered by Code Review GPT

@islxyqwe islxyqwe merged commit ff07f92 into main Jul 5, 2024
6 checks passed
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