-
Notifications
You must be signed in to change notification settings - Fork 494
Feature add disable style #916
base: master
Are you sure you want to change the base?
Feature add disable style #916
Conversation
add hour and minute
change default time to 08:00
update version
set right panel locale
Set first day of week
set default locale is enUS
change classname
change right panel click callback
Choose precise time to improve
dayjs replace moment
add esc and enter logic for time type date
fix error color
update version update version
change date input auto focus
update calendar to support time input and selection
feat(ui): update the width of year/month panel and improve style cons…
feat: update calendar ux
chore: update version
fix: keyboard handler and shortcut key
Co-authored-by: zhouwenxuan <Aries@Mac.local>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @yinjianfei-user, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily refactors the date and time handling logic by migrating from the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a major overhaul, rebranding the project from rc-calendar to @seafile/seafile-calendar, migrating from moment to dayjs, and completely redesigning the UI. It also introduces a new feature for selecting hours and minutes in a side panel. While these are significant improvements, there is a critical regression: keyboard navigation for changing dates has been removed, which severely impacts accessibility. Additionally, there's a critical bug in the new date validation logic that will fail for years after 2068. These issues must be addressed before merging.
| export function validateCalendarYear(yearStr) { | ||
| const year = yearStr; | ||
| if (!year || isNaN(year)) return getCurrentYear(); | ||
| if (year.length === 2) { | ||
| if (Number(year) >= 0 && Number(year) < 69) { | ||
| return year ? `20${year}` : getCurrentYear(); | ||
| } else if (Number(year) >= 69 && Number(year) < 100) { | ||
| return year ? `19${year}` : getCurrentYear(); | ||
| } | ||
| } | ||
| if (year.length === 4) { | ||
| return year; | ||
| } | ||
| return year ? year.padStart(4, '0') : getCurrentYear(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical Bug: Hardcoded Century Logic
The logic for handling two-digit years contains a hardcoded value (69) to determine the century. This will lead to incorrect date parsing for years after 2068 (e.g., '69' will be parsed as '1969' instead of '2069').
This should be replaced with a dynamic approach, such as a sliding window based on the current year, to avoid this Y2K-style bug.
export function validateCalendarYear(yearStr) {
const year = yearStr;
if (!year || isNaN(year)) return getCurrentYear();
if (year.length === 2) {
const currentYear = getCurrentYear();
const currentCentury = Math.floor(currentYear / 100) * 100;
let twoDigitYear = Number(year);
// Sliding window: assume year is within 80 years in the past and 20 in the future.
if (twoDigitYear > (currentYear % 100) + 20) {
twoDigitYear += currentCentury - 100;
} else {
twoDigitYear += currentCentury;
}
return String(twoDigitYear);
}
if (year.length === 4) {
return year;
}
return year ? year.padStart(4, '0') : getCurrentYear();
}| event.preventDefault(); | ||
| event.stopPropagation(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling event.preventDefault() and event.stopPropagation() in the default case of a keydown handler is generally not recommended. This will prevent all other unhandled key presses from performing their default browser action (e.g., tabbing to the next element) or propagating, which can lead to unexpected behavior. It's better to only prevent the default action for keys that you are explicitly handling.
| event.preventDefault(); | |
| event.stopPropagation(); | |
| this.props.onKeyDown(event); | |
| return 1; |
| # node version | ||
| >= 18.20.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The requirement for Node.js version >= 18.20.4 is quite specific and recent. Is this a strict requirement for the package to function? To ensure broader compatibility for developers using this library, it's generally recommended to support active LTS (Long-Term Support) versions of Node.js. Consider relaxing this requirement to a broader range, such as >=18 or the lowest compatible LTS version, unless there's a specific feature from 18.20.4 that is essential.
| componentDidMount() { | ||
| setTimeout(() => { | ||
| this.focus(); | ||
| }, 100); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using setTimeout to manage focus is often a code smell and can be unreliable. It suggests a dependency on timing that might fail under different conditions (e.g., on slower devices). A more declarative approach is to use the autoFocus prop directly on the <input> element. This makes the intent clearer and is more robust.
No description provided.