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

feat: add meta change feature for TableWalker #368

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

islxyqwe
Copy link
Member

@islxyqwe islxyqwe commented Apr 8, 2024

Make TableWalker able to change field's meta when the onMetaChange prop is set in TableWalker.
image

Copy link

vercel bot commented Apr 8, 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 Apr 8, 2024 9:48am

Copy link
Contributor

github-actions bot commented Apr 8, 2024

Risk Level 2 - /home/runner/work/graphic-walker/graphic-walker/packages/graphic-walker/src/store/visualSpecStore.ts

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. Use of magic strings: There are several instances where string literals are used directly in the code (e.g., 'Chart 1', 'bin', 'log10', etc.). These could be replaced with constants to avoid potential typos and make it easier to manage these values.
const CHART_NAME = 'Chart 1';
const BIN_TYPE = 'bin';
const LOG_TYPE = 'log10';
// use these constants in the code
  1. Long functions: Some functions like paintFields and moveField are quite long and complex. Breaking them down into smaller, more manageable functions would improve readability and make the code easier to maintain.

  2. Lack of comments: There are very few comments in the code, making it hard to understand the purpose of some functions and variables. Adding comments would greatly improve the maintainability of the code.

  3. Use of any type: There are a few instances where the any type is used. This defeats the purpose of TypeScript's static typing and can lead to runtime errors. It would be better to define interfaces or types for these instances.

  4. Error handling: There doesn't seem to be much error handling in the code. Adding error handling would make the code more robust and prevent potential issues at runtime.


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

The code changes are generally safe and follow good practices. However, there are a few areas that could be improved for better readability and maintainability:

  1. Error Handling: The reportError function is used to handle errors, but it's not clear what happens if onError is not provided. It would be better to have a default error handling behavior. For example:
onError?.(err) || console.error(err);
  1. Use of Ternary Operator: In the TableAppWithContext function, the ternary operator is used to assign values to appearance and data. This could be simplified for better readability. For example:
const appearance = props.appearance || props.dark;
const data = props.data || props.dataSource;
  1. Use of Optional Chaining: In the DatasetTable component, the onMetaChange prop is conditionally assigned based on vizStore.onMetaChange. This could be simplified using optional chaining. For example:
onMetaChange={vizStore.onMetaChange && ((fid, fIndex, diffMeta) => vizStore.updateCurrentDatasetMetas(fid, diffMeta))}
  1. Use of useMemo Hook: The useMemo hook is used in TableAppWithContext function. It's not clear why the computation is memoized. If the computation is not expensive, it might be unnecessary to use useMemo.

📚🔍🔧


Powered by Code Review GPT

@ObservedObserver ObservedObserver merged commit c8dd5ed into main Apr 8, 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