Scheduler: delete Appointment enhancements#32402
Scheduler: delete Appointment enhancements#32402sjbur wants to merge 13 commits intoDevExpress:26_1from
Conversation
| $list-item-selected-hover-bg: color.change($base-accent, $alpha: 0.3) !default; | ||
| $list-item-focused-selected-bg: color.change($base-focus-bg, $alpha: 0.7) !default; | ||
| $list-focused-bg: $base-focus-bg !default; | ||
| $list-focused-bg: $list-item-hover-bg !default; |
There was a problem hiding this comment.
I see that color is changed for the dxList. Were these changes discussed with designer and Navigation squad?
There was a problem hiding this comment.
As part of this task we needed to fix contrast within background and foreground in appointment item collector. Appointment collector item is a list item. I agree that maybe we should not change this, but in this case I am not sure that can fix contrast issue on our side
There was a problem hiding this comment.
If the contrast issue can reproduced only with dxList, then, yeah, it should be fixed here. But it's still strange that $list-focused-bg variable is assigned a value of $list-item-hover-bg. These two variables should used for different scenarios.
If the contrast issue doesn't reproduce on dxList and only on scheduler's side, then we should override some of the styles on Scheduler's side.
If you have some questions, let's discuss them :)
| this._list.registerKeyHandler?.('del', (e) => { | ||
| const { focusedElement } = this._list.option(); | ||
|
|
||
| if (!focusedElement || !this._options.allowDelete) { |
There was a problem hiding this comment.
If I have an appt with 'disabled' field, e.g:
{
disabled: true,
text: 'Website Re-Design Plan',
startDate: new Date('2021-04-26T16:30:00.000Z'),
endDate: new Date('2021-04-26T18:30:00.000Z'),
}Then this appointment can be deleted using KBN, even though it shouldn't because delete icon isn't displayed.
There was a problem hiding this comment.
Take a look at this line of code.
Instead of passing _options.allowDelete from m_scheduler, it would be better to use the existing logic. I would suggest to move that line of code into a separate function and use it in both places
There was a problem hiding this comment.
Implemented same logic as in that line
There was a problem hiding this comment.
Ok, but can we move this condition to a separate function? I think, it's better to keep such logic in one place. Or if you have objections, let's discuss it
There was a problem hiding this comment.
Also, please, add a test for the case of not allowing deletion of disabled appointment by keyboard
There was a problem hiding this comment.
Extracted to separate method
|
|
||
| if (appointment) { | ||
| e.preventDefault(); | ||
| this.hide(); |
There was a problem hiding this comment.
Was hiding a tooltip after appt is deleted using KBN discussed with the designer?
For me it feels like it shouldn't, so I would like to know what the designer thinks
There was a problem hiding this comment.
The designer suggested not to hide the collector neither by keyboard deletion neither by mouse. Hide collector only if it does not have any appointments left
There was a problem hiding this comment.
Ok, can you please add tests for scenarios when tooltip should/shouldn't close?
packages/devextreme/js/__internal/scheduler/tooltip_strategies/m_tooltip_strategy_base.ts
Outdated
Show resolved
Hide resolved
2 participants
No description provided.