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.

Sign up for GitHub

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

16700 Fixed validation in LimitedRestorationPanel component #258

Merged
merged 3 commits into from
Sep 13, 2024
Merged

16700 Fixed validation in LimitedRestorationPanel component #258

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7f76f54
- fixed input handling and validation
Sep 13, 2024
1bb16aa
- updated unit tests
Sep 13, 2024
27ba5da
- don't call validate before component is rendered (fixes unit test w…
Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 17 additions & 28 deletions src/components/limited-restoration-panel/LimitedRestorationPanel.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
hide-spin-buttons
:rules="monthsRules"
:disabled="(radioValue !== 'customMonths')"
@input="onMonthsInput($event)"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this, I now watch the v-model property (inputValue).

/>
</v-form>
<span class="ml-2 mt-2 month-text">month(s)</span>
Expand Down Expand Up @@ -84,9 +83,9 @@ export default class LimitedRestorationPanel extends Vue {

/**
* Called when the component is mounted.
* Sets the initial radio button and months input (if applicable).
*/
mounted (): void {
// set the initial radio button and months input (if applicable)
if ([24, 18, 12, 6].includes(this.months)) {
this.radioValue = this.months.toString()
} else {
Expand All @@ -96,49 +95,38 @@ export default class LimitedRestorationPanel extends Vue {
}

/**
* Called when months input has changed.
* Called when radio value or input value have changed.
*/
async onMonthsInput (input: string): Promise<void> {
// ignore non-inputs
if (input !== null) {
// wait for component updates
@Watch('radioValue')
@Watch('inputValue')
private async onValueChanged (): Promise<void> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous 2 methods fired alternately and not always in the same order due to waiting for the input component to update/validate.

This now handles things more cleanly.

if (this.radioValue === 'customMonths') {
// wait for component updates then validate the input field
await this.$nextTick()
// validate the input
const valid = this.$refs.monthsRef.validate()
// emit validity

// emit months and validity
this.emitMonths(valid ? +this.inputValue : null) // emit null if invalid
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

Copy link
Collaborator Author

@severinbeauvais severinbeauvais Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Yes, this way this component always emits something reasonable without events depending on other events (eg, only emit month if valid).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other reason I did this is that it allows the parent component to do something different if invalid. In this case (next PR), the parent will clear any existing expiry date.

this.emitValid(valid)
// if valid, emit new value
if (valid) this.emitMonths(Number(this.inputValue))
}
}
} else {
// reset input field validation
this.$refs.monthsRef.resetValidation()

/**
* Called when radio button has changed.
*/
@Watch('radioValue')
private onRadioValueChanged () {
if (this.radioValue !== 'customMonths') {
// reset months text field when another radio button is selected
this.$refs.monthsRef.reset()
// emit months and validity
this.emitMonths(Number(this.radioValue))
this.emitMonths(+this.radioValue)
Copy link
Collaborator Author

@severinbeauvais severinbeauvais Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unary plus operator converts a string to a number more cleanly, imho.

this.emitValid(true)
} else if (!this.inputValue) {
// as there is no input value, this component is invalid
// otherwise, validation will be done in onMonthsInput()
this.emitValid(false)
}
}

/**
* Emits the numbed of months selected.
* Emits the number of months selected.
*/
@Emit('months')
// eslint-disable-next-line @typescript-eslint/no-unused-vars
private emitMonths (months: number): void {}

/**
* Emits whether the number of months selected is valid.
* Emits the component validity.
*/
@Emit('valid')
// eslint-disable-next-line @typescript-eslint/no-unused-vars
Expand All @@ -148,6 +136,7 @@ export default class LimitedRestorationPanel extends Vue {

<style lang="scss" scoped>
@import '@/assets/styles/theme.scss';

.month-text {
color: $gray7;
}
Expand Down
35 changes: 17 additions & 18 deletions tests/unit/LimitedRestorationPanel.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,25 +27,23 @@ function createDefaultComponent (

describe('Initialize RelationshipsPanel component', () => {
it('loads the component', () => {
const wrapper: Wrapper<LimitedRestorationPanel> = createDefaultComponent()
const wrapper = createDefaultComponent()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createDefaultComponent() already returns a type.


expect(wrapper.findComponent(LimitedRestorationPanel).exists()).toBe(true)

wrapper.destroy()
})

it('loads with a preset expiry (24 months)', async () => {
const wrapper: Wrapper<LimitedRestorationPanel> = createDefaultComponent(24)
await Vue.nextTick()
it('loads with a preset expiry (24 months)', () => {
const wrapper = createDefaultComponent(24)

expect(wrapper.vm.$data.radioValue).toEqual('24')

wrapper.destroy()
})

it('loads with a custom expiry (1 month)', async () => {
const wrapper: Wrapper<LimitedRestorationPanel> = createDefaultComponent(1)
await Vue.nextTick()
it('loads with a custom expiry (1 month)', () => {
const wrapper = createDefaultComponent(1)

expect(wrapper.vm.$data.radioValue).toEqual('customMonths')
expect(wrapper.vm.$data.inputValue).toEqual('1')
Expand All @@ -54,48 +52,49 @@ describe('Initialize RelationshipsPanel component', () => {
})

it('emits events when we select a preset expiry (24 months)', async () => {
const wrapper: Wrapper<LimitedRestorationPanel> = createDefaultComponent()
await Vue.nextTick()
const wrapper = createDefaultComponent()

await wrapper.find('#radio-24').setChecked()

expect(wrapper.emitted('valid').pop()[0]).toBe(true)
expect(wrapper.emitted('months').pop()[0]).toEqual(24)

wrapper.destroy()
})

it('emits events when we select a custom expiry (1 month)', async () => {
const wrapper: Wrapper<LimitedRestorationPanel> = createDefaultComponent()
await Vue.nextTick()
const vm = wrapper.vm as any
const wrapper = createDefaultComponent()

await wrapper.find('#radio-custom').setChecked()
await wrapper.find('#text-field-months').setValue('1')
await Vue.nextTick()

expect(wrapper.emitted('valid').pop()[0]).toBe(true)
expect(wrapper.emitted('months').pop()[0]).toEqual(1)

wrapper.destroy()
})

it('emits valid=false when we select 25 months with a max of 24', async () => {
const wrapper: Wrapper<LimitedRestorationPanel> = createDefaultComponent()
await Vue.nextTick()
const vm = wrapper.vm as any
const wrapper = createDefaultComponent()

await wrapper.find('#radio-custom').setChecked()
await wrapper.find('#text-field-months').setValue('25')

expect(wrapper.emitted('valid').pop()[0]).toEqual(false)
expect(wrapper.emitted('months').pop()[0]).toEqual(null)

wrapper.destroy()
})

it('emits valid=true when we select 25 months with a max of 36', async () => {
const wrapper: Wrapper<LimitedRestorationPanel> = createDefaultComponent(undefined, 36)
await Vue.nextTick()
const vm = wrapper.vm as any
const wrapper = createDefaultComponent(undefined, 36)

await wrapper.find('#radio-custom').setChecked()
await wrapper.find('#text-field-months').setValue('25')

expect(wrapper.emitted('valid').pop()[0]).toEqual(true)
expect(wrapper.emitted('months').pop()[0]).toEqual(25)

wrapper.destroy()
})
Expand Down
Loading