Scheduler: Encapsulate old view model and add collector tests#30861
Scheduler: Encapsulate old view model and add collector tests#30861Ambrozy merged 6 commits intoDevExpress:25_2from Ambrozy:25_2_collector_tests
Conversation
Ambrozy
changed the title
| import type { AppointmentItemViewModel } from './view_model/generate_view_model/types'; | ||
|
|
||
| const toMs = dateUtils.dateToMilliseconds; | ||
| const VERTICAL_VIEW_TYPES = ['day', 'week', 'workWeek']; |
There was a problem hiding this comment.
I noticed this string is repeated in a couple of places. (in packages/devextreme/js/__internal/scheduler/appointments/resizing/get_delta_time.ts )
Maybe we could extract it as a constant to avoid duplication?
| case VERTICAL_VIEW_TYPES.includes(viewType) && !isAllDay: | ||
| return getVerticalDeltaTime(args, initialSize, options); | ||
| default: | ||
| return isAllDay |
There was a problem hiding this comment.
I think you can use case isAllDay: instead of the ternary operation
so you don’t need to branch further inside the default.
| const APPOINTMENT_COLLECTOR_CONTENT_CLASS = `${APPOINTMENT_COLLECTOR_CLASS}-content`; | ||
|
|
||
| const WEEK_VIEW_COLLECTOR_OFFSET = 5; | ||
| const COMPACT_THEME_WEEK_VIEW_COLLECTOR_OFFSET = 1; |
There was a problem hiding this comment.
I noticed that WEEK_VIEW_COLLECTOR_OFFSET and COMPACT_THEME_WEEK_VIEW_COLLECTOR_OFFSET were removed and are no longer used.
How is this offset handled now? Could this change affect the behavior in week view or in compact theme scenarios?
| } | ||
| }, () => { | ||
| QUnit.test('_collectorOffset option should be passed to SchedulerAppointments depending on the view', async function(assert) { | ||
| QUnit.test('_collectorOffset option should be depending on the view', async function(assert) { |
There was a problem hiding this comment.
I think these tests for Material-based themes are not very useful.
They only check a private option passed down to appointments, but I couldn’t find any unit tests that cover this option in the collection appointments logic itself.
May be we can delete him? If we had integration tests
| const appointments = scheduler.instance.getAppointmentsInstance(); | ||
|
|
||
| assert.equal(appointments.option('_collectorOffset'), 20, 'SchedulerAppointments has correct _collectorOffset'); | ||
| assert.equal(scheduler.instance.getCollectorOffset(), 20, 'SchedulerAppointments has correct _collectorOffset'); |
There was a problem hiding this comment.
I think we should change assert name, or remove this test
2 participants
No description provided.