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 iso year and week feature #386

Merged
merged 6 commits into from
May 26, 2024
Merged

Conversation

islxyqwe
Copy link
Member

add the iso feature of temporal fields.
image

Copy link

vercel bot commented May 21, 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 May 24, 2024 0:28am

Copy link
Contributor

github-actions bot commented May 21, 2024

Risk Level 2 - /home/runner/work/graphic-walker/graphic-walker/packages/graphic-walker/src/lib/vega.ts

The code is generally well-written, but there are a few areas where improvements can be made for readability and maintainability.

  1. The toVegaSpec function is quite long and complex. Consider breaking it down into smaller, more manageable functions. This would improve readability and maintainability.

  2. There are several instances where you are using NULL_FIELD as a default value. Consider creating a constant for this default value to avoid potential typos and make the code easier to understand.

  3. The if (layoutMode === 'auto') condition is empty. If this is intentional, consider adding a comment explaining why. If it's not, consider removing it or adding the necessary code.

  4. The for loop at the end of the function could be replaced with a map function for better readability. For example:

let result = rowRepeatFields.map((rowField, i) => {
    return colRepeatFields.map((colField, j) => {
        // ...existing code...
    });
}).flat();

This will make your code more idiomatic and easier to understand.


Risk Level 2 - /home/runner/work/graphic-walker/graphic-walker/packages/graphic-walker/src/components/dropdownSelect/combobox.tsx

The code is generally well written, but there is a potential issue with the onSelect function. It seems to be setting the selected value to an empty string if the current value is selected again. This could potentially lead to unexpected behavior if the empty string is not handled properly elsewhere in the code. Consider revising this behavior or ensure that the empty string is handled correctly.

onSelect={() => {
    if (opt.value === '_none') {
        onSelect?.('');
    } else {
        onSelect?.(opt.value === selectedKey ? '' : opt.value);
    }
    setOpen(false);
}}

Risk Level 3 - /home/runner/work/graphic-walker/graphic-walker/packages/graphic-walker/src/lib/op/dateTimeDrill.ts

The code is generally well written, but there is a potential issue with the iso_year case in the switch statement. The logic seems to be complex and could potentially lead to bugs or issues in the future. Consider simplifying this logic or adding more comments to explain what it is doing.

case 'iso_year': {
    const newValues = fieldValues.map((v) => {
        const date = newDate(v);
        const _Y = date.getFullYear();
        const dayInFirstWeek = toOffsetDate(_Y, 0, 4);
        const firstMondayOfYear = newDate(newDate(dayInFirstWeek).setDate(dayInFirstWeek.getDate() - (dayInFirstWeek.getDay() || 7) + 1));
        if (date.getTime() < firstMondayOfYear.getTime()) {
            return formatDate(toOffsetDate(_Y - 1, 0, 1));
        }
        const nextDayInFirstWeek = toOffsetDate(_Y + 1, 0, 4);
        const nextFirstMondayOfYear = newDate(
            newDate(nextDayInFirstWeek).setDate(nextDayInFirstWeek.getDate() - (nextDayInFirstWeek.getDay() || 7) + 1)
        );
        return formatDate(toOffsetDate(date.getTime() < nextFirstMondayOfYear.getTime() ? _Y : _Y + 1, 0, 1));
    });
    return {
        ...data,
        [resKey]: newValues,
    };
}

📚🔧🔍


Powered by Code Review GPT

@ObservedObserver ObservedObserver merged commit caf237a into main May 26, 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