-
Notifications
You must be signed in to change notification settings - Fork 50
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
Usability improvements around the picker components #3172
Merged
+2,683
−1,857
Merged
Changes from 32 commits
Commits
Show all changes
37 commits
Select commit
Hold shift + click to select a range
a6f2499
Replace deprecated slot syntax
carlobeltrame 46657bf
Fix unusual translation
carlobeltrame a8c22fb
Refactor and simplify pickers to be more user friendly
carlobeltrame e11a7a3
Allow closing pickers using the escape key
carlobeltrame 39a9710
Disallow selecting invalid date and time combinations
carlobeltrame 54a2a06
Take the locale into account when validating time inputs
carlobeltrame 131fe4a
Adjust tests to removed buttons
carlobeltrame 2e251cf
Validate value when changed, not when clicking anywhere on the page
carlobeltrame 1ade19f
Also allow shorter versions of manually entered dates in picker
carlobeltrame 6e034ba
Clean up handling of the many values in BasePicker
carlobeltrame b9d4c24
Correct documentation comments
carlobeltrame 3192c9e
Add aria label to picker open buttons
carlobeltrame aee1ca8
Translate format validation error
carlobeltrame 7bb0974
Rewrite date picker specs using testing library
carlobeltrame e03afb5
Reuse code, follow recommendations of VTL
carlobeltrame f4e3036
Improve aria text wording
carlobeltrame 272ec2c
Rewrite time picker specs using testing library
carlobeltrame 72e32e0
Rewrite color picker tests using testing library
carlobeltrame ea7826b
Improve accessibility and testability of button components
carlobeltrame acbf28d
Avoid double submit when clicking save button
carlobeltrame 0073b26
The reload button should not submit the form field
carlobeltrame 303e663
Rewrite api date picker tests using testing library
carlobeltrame 885dd8e
Rewrite api time picker tests using testing library
carlobeltrame 20c96b9
Rewrite api color picker tests using testing library
carlobeltrame d829bd0
Rework accessibility of generic button components
carlobeltrame f9aa961
Add tests for opening and closing the pickers
carlobeltrame 55bb7b1
Add a test for supporting short date notation
carlobeltrame f3720de
Extract and unit test our date/time validations
carlobeltrame 7f8e1f9
Display days of adjacent months in date picker, for easier selection …
carlobeltrame b079395
Initialize veeValidate after dayjs, to make sure dayjs is available f…
carlobeltrame 1ae28c0
Update tests after newly displaying adjacent months' days
carlobeltrame b0199cc
Try to display a useful month when opening the date picker
carlobeltrame 2ebe629
Reactivate the capability to move schedule entries to different perio…
carlobeltrame 7dcc984
Merge remote-tracking branch 'origin/devel' into picker-fixes
carlobeltrame a407ffd
Simplify picker closing behaviour
carlobeltrame bb452e1
Use 12 hour AM/PM format in time picker depending on locale
carlobeltrame 4eb65ff
Fix time greater_than validation for different locales
carlobeltrame File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
125 changes: 57 additions & 68 deletions
125
frontend/src/components/form/api/__tests__/ApiColorPicker.spec.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,101 +1,90 @@ | ||
import ApiColorPicker from '../ApiColorPicker' | ||
import ApiWrapper from '@/components/form/api/ApiWrapper' | ||
import Vue from 'vue' | ||
import Vuetify from 'vuetify' | ||
import flushPromises from 'flush-promises' | ||
import formBaseComponents from '@/plugins/formBaseComponents' | ||
import merge from 'lodash/merge' | ||
import { screen, waitFor } from '@testing-library/vue' | ||
import { render } from '@/test/renderWithVuetify.js' | ||
import user from '@testing-library/user-event' | ||
import { ApiMock } from '@/components/form/api/__tests__/ApiMock' | ||
import { extend } from 'vee-validate' | ||
import { i18n } from '@/plugins' | ||
import { mount as mountComponent } from '@vue/test-utils' | ||
import { regex } from 'vee-validate/dist/rules' | ||
import { waitForDebounce } from '@/test/util' | ||
|
||
Vue.use(Vuetify) | ||
Vue.use(formBaseComponents) | ||
|
||
extend('regex', regex) | ||
|
||
describe('An ApiColorPicker', () => { | ||
let vuetify | ||
let wrapper | ||
let apiMock | ||
|
||
const fieldName = 'test-field/123' | ||
const COLOR_1 = '#ff0000' | ||
const COLOR_2 = '#ff00ff' | ||
const FIELD_NAME = 'test-field/123' | ||
const FIELD_LABEL = 'Test field' | ||
const COLOR_1 = '#FF0000' | ||
const COLOR_2 = '#FAFFAF' | ||
const PICKER_BUTTON_LABEL_TEXT = 'Dialog öffnen um eine Farbe für Test field zu wählen' | ||
|
||
beforeEach(() => { | ||
vuetify = new Vuetify() | ||
apiMock = ApiMock.create() | ||
}) | ||
|
||
afterEach(() => { | ||
jest.restoreAllMocks() | ||
wrapper.destroy() | ||
}) | ||
|
||
const mount = (options) => { | ||
const app = Vue.component('App', { | ||
components: { ApiColorPicker }, | ||
test('triggers api.patch and status update if input changes', async () => { | ||
// given | ||
apiMock.get().thenReturn(ApiMock.success(COLOR_1).forFieldName(FIELD_NAME)) | ||
apiMock.patch().thenReturn(ApiMock.success(COLOR_2)) | ||
const { container } = render(ApiColorPicker, { | ||
props: { | ||
fieldName: { type: String, default: fieldName }, | ||
autoSave: false, | ||
fieldname: FIELD_NAME, | ||
uri: 'test-field/123', | ||
label: FIELD_LABEL, | ||
required: true, | ||
}, | ||
template: ` | ||
<div data-app> | ||
<api-color-picker | ||
:auto-save="false" | ||
:fieldname="fieldName" | ||
uri="test-field/123" | ||
label="Test field" | ||
required="true" | ||
/> | ||
</div> | ||
`, | ||
}) | ||
apiMock.get().thenReturn(ApiMock.success(COLOR_1).forFieldName(fieldName)) | ||
const defaultOptions = { | ||
mocks: { | ||
$tc: () => {}, | ||
api: apiMock.getMocks(), | ||
}, | ||
} | ||
return mountComponent(app, { | ||
vuetify, | ||
i18n, | ||
attachTo: document.body, | ||
...merge(defaultOptions, options), | ||
}) | ||
} | ||
|
||
test('triggers api.patch and status update if input changes', async () => { | ||
apiMock.patch().thenReturn(ApiMock.success(COLOR_2)) | ||
wrapper = mount() | ||
|
||
await flushPromises() | ||
|
||
const input = wrapper.find('input') | ||
await input.setValue(COLOR_2) | ||
await input.trigger('submit') | ||
|
||
await waitForDebounce() | ||
await flushPromises() | ||
|
||
expect(apiMock.getMocks().patch).toBeCalledTimes(1) | ||
expect(wrapper.findComponent(ApiWrapper).vm.localValue).toBe(COLOR_2) | ||
// when | ||
// click the button to open the picker | ||
await user.click(screen.getByLabelText(PICKER_BUTTON_LABEL_TEXT)) | ||
// click inside the color picker canvas to select a different color | ||
const canvas = container.querySelector('canvas') | ||
await user.click(canvas, { clientX: 10, clientY: 10 }) | ||
// click the save button | ||
await user.click(screen.getByLabelText('Speichern')) | ||
|
||
// then | ||
await waitFor(async () => { | ||
const inputField = await screen.findByLabelText(FIELD_LABEL) | ||
expect(inputField.value).toBe(COLOR_2) | ||
expect(apiMock.getMocks().patch).toBeCalledTimes(1) | ||
}) | ||
}) | ||
|
||
test('updates state if value in store is refreshed and has new value', async () => { | ||
wrapper = mount() | ||
apiMock.get().thenReturn(ApiMock.success(COLOR_2).forFieldName(fieldName)) | ||
|
||
wrapper.findComponent(ApiWrapper).vm.reload() | ||
// given | ||
apiMock.get().thenReturn(ApiMock.networkError().forFieldName(FIELD_NAME)) | ||
render(ApiColorPicker, { | ||
props: { | ||
autoSave: false, | ||
fieldname: FIELD_NAME, | ||
uri: 'test-field/123', | ||
label: FIELD_LABEL, | ||
required: true, | ||
}, | ||
mocks: { | ||
api: apiMock.getMocks(), | ||
}, | ||
}) | ||
await screen.findByText('A network error occurred.') | ||
expect((await screen.findByLabelText(FIELD_LABEL)).value).not.toBe(COLOR_1) | ||
const retryButton = await screen.findByText('Erneut versuchen') | ||
apiMock.get().thenReturn(ApiMock.success(COLOR_1).forFieldName(FIELD_NAME)) | ||
|
||
await waitForDebounce() | ||
await flushPromises() | ||
// when | ||
await user.click(retryButton) | ||
|
||
expect(wrapper.findComponent(ApiWrapper).vm.localValue).toBe(COLOR_2) | ||
expect(wrapper.find('input[type=text]').element.value).toBe(COLOR_2) | ||
// then | ||
await waitFor(async () => { | ||
expect((await screen.findByLabelText(FIELD_LABEL)).value).toBe(COLOR_1) | ||
}) | ||
}) | ||
}) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This one depends a bit on how we progress in #3127
Depending on the edit mode this can/cannot make sense
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.
Absolutely. I just wanted to leave the code in a consistent state, no matter what the decision in #3127 comes out to be. I didn't want to introduce client-side validation on period create, but not on update.