Conversation
It will emit an event to get the textual input value, but the main model is a Date object.
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
|
✅ Deploy Preview for histoire-site ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for histoire-controls ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for histoire-examples-vue3 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary of ChangesHello @Kapcash, 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 a new HstDate control component to the Histoire library, providing a much-needed solution for handling Date object instances within component props. Previously, developers had to resort to workarounds with HstText for date inputs, which was not ideal due to limitations in input type enforcement and proper date validation. The new HstDate component offers a native date/datetime picker experience, emitting both a Date object and its string value, and supports toggling between date-only and datetime-local input types. This enhancement streamlines the process of integrating date-related controls into Histoire stories. 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
|
✅ Deploy Preview for histoire-examples-svelte3 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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 a new HstDate component for selecting dates and datetimes. This is a great addition and the implementation is solid, including comprehensive tests and a story file. My review focuses on a few key areas to improve robustness and clarity:
- I've identified a reactivity issue in
HstDate.vuewhere the component doesn't update when itsmodelValueprop changes, and I've suggested a fix using awatcheffect. - I've also suggested adding a unit test to cover this reactivity scenario to prevent future regressions.
- Finally, I've provided feedback on the story file to improve the clarity of the controls for demonstration purposes.
Overall, this is a well-executed feature. Great work on your first contribution!
| </script> | ||
|
|
||
| <script lang="ts" setup> | ||
| import { computed, ref } from 'vue' |
There was a problem hiding this comment.
To make the component reactive to prop changes, you'll need to import watch from vue.
import { computed, ref, watch } from 'vue'
| let initialValue = '' | ||
| if (props.modelValue instanceof Date) { | ||
| initialValue = props.datetime | ||
| ? props.modelValue.toISOString().slice(0, 16) | ||
| : props.modelValue.toISOString().slice(0, 10) | ||
| } | ||
| const inputValue = ref(initialValue) |
There was a problem hiding this comment.
The component's internal inputValue is initialized from props.modelValue only once during setup. This means if the modelValue or datetime props are updated by the parent component after the initial render, the change will not be reflected in the input field, leading to a UI state that is out of sync with the component's props.
To fix this, you should use a watch effect to react to changes in these props and update inputValue accordingly. This ensures the component remains synchronized with its props throughout its lifecycle.
const inputValue = ref('')
watch([() => props.modelValue, () => props.datetime], ([newDate, datetime]) => {
let value = ''
if (newDate instanceof Date) {
value = datetime
? newDate.toISOString().slice(0, 16)
: newDate.toISOString().slice(0, 10)
}
if (value !== inputValue.value) {
inputValue.value = value
}
}, { immediate: true })
| const input = wrapper.find('input') | ||
| expect(input.element.value).toBe('') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The test suite is quite comprehensive, which is great! However, it's missing a test case for reactivity. The component should update its displayed value when the modelValue prop is changed externally after the initial mount. Adding a test for this scenario would have caught a reactivity bug in the component and will help prevent future regressions.
You can add a test like this inside the 'modelValue initialization' describe block:
it('updates input value when modelValue prop changes', async () => {
const wrapper = mount(HstDate, {
props: {
modelValue: new Date('2024-12-25'),
},
})
const input = wrapper.find('input')
expect(input.element.value).toBe('2024-12-25')
await wrapper.setProps({ modelValue: new Date('2025-01-01') })
expect(input.element.value).toBe('2025-01-01')
})| <HstText | ||
| v-model="state.dateText" | ||
| title="Text" | ||
| /> |
There was a problem hiding this comment.
In the story controls, you have an HstText input bound to state.dateText. This allows editing the string representation of the date, but it doesn't update the HstDate component's value (state.date). This can be confusing as it seems like a two-way binding, but it only works one way (from HstDate to dateText).
To avoid this confusion, you could either:
- Make the
HstTextinputreadonlyto just display the emitted string value. - Implement a
watchin the story's setup to parsestate.dateTextand updatestate.date, effectively creating a two-way sync for demonstration purposes.
Given the component's design focuses on the Date model, making the text display read-only might be the clearer option.
histoire
@histoire/app
@histoire/controls
@histoire/plugin-nuxt
@histoire/plugin-percy
@histoire/plugin-screenshot
@histoire/plugin-svelte
@histoire/plugin-vue
@histoire/shared
@histoire/vendors
commit: |
hugoattal
left a comment
hugoattal
left a comment
There was a problem hiding this comment.
A few ideas on the component implementation 🤔 (Thanks a lot for contributing ❤️)
It duplicates the native input event: `@input="$event.target.value"`
2 participants
Description
Use case
In some cases, we need to pass Date object instance to component props. But we lack support for native Histoire controllers for selecting dates.
I personally need this on my company's Histoire environment, and I had to tweak a
HstTextcomponent to validate only date texts, convert it to date instance, and make sure it doesn't break anything.Since
HstTextenforces the inputtypeto betext(thev-bindis overridden), we can't use it withtype="date". And I thought we probably shouldn't, as it would make the component too versatile. It would not handle edge cases, validation and so on.Solution
So I implemented this new controller component
HstDateto select a Date. It's a native input date / datetime picker, which emits the Date instance object, and also the textual value hold by the<input>element.My first thought was to support both
Dateandstringmodels, both synced together. But it brings a whole lot of issues, especially if we use both models at the same time (which one takes precedence?). So I decided to focus on theDatemodel instead, which is I think the main use case we would use this controller component.Additional context
Since it's my first contribution, I'm not sure if this component respects the repo best practices and code style.
I also need feedback over the component naming and its story organization.
Should I add some examples in the
examplespackage?What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).