Scheduler - Appointment Form - Fix KBN issues #32225
Conversation
| private createRecurrenceRuleGroup(): GroupItem { | ||
| // Change of frequency editor's value causes rerender of the recurrencePatternGroup. | ||
| // To prevent focus loss in this editor, we use this flag. | ||
| let needRestoreFrequencyEditorFocus = false; |
There was a problem hiding this comment.
The reason of focus loss is because recurrencePatternGroup group gets rerendered when updateDayEditorsVisibility() is called.
Another solution is to wrap weekGroup, monthGroup and yearGroup into different group here.
I have decided to use this flag, because it's used in very narrow scope and because form items already have very deep nesting (docs).
But if you have any objections, let's discuss it :)
| await testScreenshot( | ||
| t, | ||
| takeScreenshot, | ||
| 'scheduler__appointment__recurrence-settings-button__not-focused.png', | ||
| { element: appointmentPopup.contentElement }, | ||
| ); |
There was a problem hiding this comment.
I think, there's no need to screenshot the form before focusing settings button, because there are already visual tests for form that do that.
| }, | ||
| })); | ||
|
|
||
| test.meta({ browserSize: [1500, 1500] })('Recurrence settings button should have correct focus state', async (t) => { |
There was a problem hiding this comment.
I suggest to move this test to form.visual.ts file, because it takes screenshot of main form, not recurrence form
| { element: appointmentPopup.contentElement }, | ||
| ); | ||
|
|
||
| await t.hover(appointmentPopup.recurrence.settingsButton); |
There was a problem hiding this comment.
It's better to focus the button using keyboard navigation, because hovering the button doesn't set it to 'focused' state.
There was a problem hiding this comment.
OK, will replace with await t.pressKey('tab')
| }); | ||
| }); | ||
|
|
||
| test('FrequencyEditor should not be focused when value is changed via API', async (t) => { |
There was a problem hiding this comment.
Can we move frequency editor tests to jest? It seems that they are purely functional. We can simulate keyboard or mouse by emitting native event. Also I think that maybe we only need keyboard tests, because both mouse and keyboard emit native events. So to me it seems that we just need one of these tests.
What do you think?
|
|
||
| const SCHEDULER_SELECTOR = '#container'; | ||
|
|
||
| test('Subject text editor should have focus after returning from recurrence form', async (t) => { |
There was a problem hiding this comment.
I think we need to place tests to move from main form to recurrence form and vice versa into a single file, since these tests are related. Maybe just have create one file for form functional testcafe tests? Like form.functional.ts?
There was a problem hiding this comment.
And if we will have new testcafe functional tests in the future? We assume that the amount of functional tests will be so low that it will not be a problem?
| &.dx-button-mode-text.dx-state-focused, | ||
| &.dx-button-mode-text.dx-state-hover { | ||
| background-color: rgba(0, 0, 0, .08); | ||
| } |
There was a problem hiding this comment.
The problem with this approach is that if background-color of dxButton is changed (for example, the designer decided to change it or new color scheme is added, or a user customizes button background color), then the background-color of settings button will not have the same background-color and we will need to change this value too.
Please address this issue. Possible solutions:
- set dx-state-press class to button when the button is focused
- set outline to the button when it's focused
There was a problem hiding this comment.
Wrote directly to you in Teams about this
There was a problem hiding this comment.
Added default outline styling like in another parts of project
2 participants
No description provided.