feat(datetime-input): add component #IX-3628 #2359
Conversation
🦋 Changeset detectedLatest commit: ba9b20b The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello @alexkaduk, 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 introduces the Highlights
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the new ix-datetime-input component, which is a significant and well-executed feature. The implementation is solid, following existing patterns from other input components, and includes comprehensive documentation and tests. I've identified one critical bug in the minDate/maxDate validation logic that could lead to incorrect behavior at date boundaries. Additionally, I've found some minor inconsistencies in the supporting documentation files that should be updated to align with the final implementation. I've also suggested an additional test case to cover the maxDate boundary condition. Overall, this is excellent work, and the suggested fixes are straightforward.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/core/src/components/datetime-input/datetime-input.tsx (256-257)
The validation for minDate and maxDate is not robust. The parseDate helper method is too lenient for parsing these properties, as it tries both the combined date-time format and the date-only format. According to the JSDoc, minDate and maxDate are expected to be date-only strings matching dateFormat.
This can lead to incorrect validation. For example, if maxDate is '2024/01/20', it will be parsed as 2024-01-20T00:00:00.000. A user input of 2024/01/20 10:00:00 would then be considered invalid because it's after midnight on that day.
To ensure the entire day is included in the valid range, you should parse minDate and maxDate strictly with dateFormat and adjust them to the start and end of the day, respectively.
const minDateTime = this.minDate
? DateTime.fromFormat(this.minDate, this.dateFormat, {
locale: this.locale,
})?.startOf('day')
: null;
const maxDateTime = this.maxDate
? DateTime.fromFormat(this.maxDate, this.dateFormat, {
locale: this.locale,
})?.endOf('day')
: null;
packages/core/src/components/datetime-input/test/datetime-input.ct.ts (274)
The test suite is missing a test case for the maxDate boundary condition. The current implementation has a bug where any time on the maxDate itself is considered invalid. Adding a test case for this boundary will help verify the fix.
I suggest adding the following test case after this line:
regressionTest('validates maxDate with date boundary', async ({ mount, page }) => {
await mount(`
<ix-datetime-input
value="2024/12/20 10:00:00"
max-date="2024/12/20"
></ix-datetime-input>
`);
const input = page.locator('ix-datetime-input input');
await expect(input).not.toHaveClass(/is-invalid/);
});DATETIME_INPUT_IMPLEMENTATION_STEPS.md (567-575)
This implementation guide includes showTimeReference and timeReference props, but they are not present in the final datetime-input.tsx implementation (and are correctly omitted according to DATETIME_INPUT_PROPS_REFERENCE.md). To maintain consistency and avoid confusion for future developers, this document should be updated to reflect the final implementation by removing these props. The usage of these props in Step 4.6 and their documentation in Step 8.2 should also be removed.
DATETIME_INPUT_INVESTIGATION.md (294-295)
The props showTimeReference and timeReference are included in this API design, but they were correctly omitted from the final implementation as noted in DATETIME_INPUT_PROPS_REFERENCE.md. To avoid confusion, this investigation document should be updated to reflect the final decision to exclude these props.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the new ix-datetime-input component, a valuable addition that combines date and time input. The implementation is thorough, including wrappers for Angular, React, and Vue, as well as comprehensive component and visual regression tests. My review focuses on enhancing the user experience and code quality of the core component. I've identified opportunities for improvement regarding the behavior of readonly inputs, handling of invalid user input to prevent state loss in the picker, and clarification of documentation. Addressing these points will further improve the robustness and usability of this new component.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ix-datetime-input component, a new feature that combines date and time input with an integrated picker. The implementation is well-structured, leveraging existing components like ix-datetime-picker and ix-field-wrapper. The component is form-ready and includes comprehensive validation for format, min/max dates, and required fields.
My review focuses on improving code consistency and removing development artifacts. I've pointed out a couple of console.warn statements that should be removed from the production build and an inconsistency in using null vs undefined for state properties.
The addition of extensive component tests is excellent and covers many use cases, ensuring the component's reliability. Overall, this is a solid contribution.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the new ix-datetime-input component, a significant and well-implemented feature. The changes span across the core component, framework wrappers (Angular, React, Vue), and include comprehensive tests. The implementation is robust, especially the extensive test suite. My review focuses on enhancing the form integration and ensuring the validation state is correctly managed and reported, which is critical for a form-associated component.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the new ix-datetime-input component, a combined date and time input field. The implementation is very thorough, including the core web component, wrappers for Angular, React, and Vue, and a comprehensive set of unit, component, and visual regression tests. The code is well-structured and follows the existing patterns in the repository. I've identified one high-severity issue regarding the user experience when invalid data is entered, which I believe should be addressed. Overall, this is an excellent contribution.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the new ix-datetime-input component, a combined date and time input field with an integrated picker. The implementation is thorough, covering the core Stencil component, framework wrappers for Angular, React, and Vue, and a comprehensive set of tests including component and visual regression tests. The component's logic for parsing, validation, and state management is well-handled. I have one suggestion to improve the consistency of event emissions.
feat(datetime-input): add component unit tests feat(datetime-input): add component tests
alexkaduk
force-pushed
the
feature/IX-3861-datetime-input
branch
from
4a7c13c to
6e7bbd4
Compare
|
💡 What is the current behavior?
No datetime input component exists. Users must use separate date and time inputs.
🆕 What is the new behavior?
Added
ix-datetime-inputcomponent - a combined date and time input with integrated picker.Features:
ix-datetime-pickerwith auto-open/close behaviordateFormat + timeFormat🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)Related docu PR is siemens/ix-docs#165