Opened 7 years ago

Closed 7 years ago

#14407 closed enhancement (fixed)

[patch][cla] CalendarLite - updateUi only when changing month/year

Reported by: Mathevet julien Owned by: bill
Priority: high Milestone: 1.8
Component: Dijit Version: 1.7.0
Keywords: Calendar CalendarLite Cc:
Blocked By: Blocking:

Description

Hi, this patch only refill day grid, only on changing month

Attachments (10)

patchUpdateGridOnChange.diff (1.1 KB) - added by Mathevet julien 7 years ago.
patchUpdateGridOnChange.2.diff (925 bytes) - added by Mathevet julien 7 years ago.
better one
patchUpdateGridOnChange.3.diff (3.3 KB) - added by Mathevet julien 7 years ago.
grid update on month change and change selected date if previous selected cell is different
patchUpdateGridOnChange.4.diff (8.3 KB) - added by Mathevet julien 7 years ago.
This one is extendable
patchTestCalendar.diff (9.3 KB) - added by Mathevet julien 7 years ago.
patchUpdateGridOnChange.5.diff (6.1 KB) - added by Mathevet julien 7 years ago.
patchUpdateGridOnChange.6.diff (6.5 KB) - added by Mathevet julien 7 years ago.
patchUpdateGridOnChange.7.diff (5.9 KB) - added by Mathevet julien 7 years ago.
patchUpdateGridOnChange.8.diff (6.6 KB) - added by Mathevet julien 7 years ago.
extract markSelected from focus
patchUpdateGridOnChange9.patch (8.0 KB) - added by bill 7 years ago.
updated patch, Calendar_a11y.patch passes regression w/out any changes, but I wonder if it's worth it to have a _markSelectedDates() method rather than leaving that logic in _populateGrid()

Download all attachments as: .zip

Change History (22)

Changed 7 years ago by Mathevet julien

Changed 7 years ago by Mathevet julien

better one

comment:1 Changed 7 years ago by bill

Looks like the code to change selection (without redrawing the grid) should happen in _setValueAttr(). _setCurrentFocusAttr() shouldn't change the selection.

Changed 7 years ago by Mathevet julien

grid update on month change and change selected date if previous selected cell is different

comment:2 Changed 7 years ago by Mathevet julien

Note: try to change patch in thinking about #9679 and #14376

comment:3 Changed 7 years ago by bill

Notes from IRC chat. I think you should:

  • get rid of _isSelectedDate(), and code in _populateGrid() that calls it
  • create a function like _setSelectedDate() to clear the old selected date and mark the new one (similar to _onChange() in your patch)
  • _setSelectedDate() would be called at the end of _setValueAttr(), and also when the calendar is switched from one month to another

_setSelectedDate() would look something like

if(this._selectedCell){
   domClass.remove(this._selectedCell, "dijitCalendarSelectedDate")
   this._selectedCell.setAttribute("aria-selected", "false"); 
}
if(newCell){
   domClass.add(newCell "dijitCalendarSelectedDate")
   this._selectedCell.setAttribute("aria-selected", "true");
}
this._selectedCell = newCell; 
Last edited 7 years ago by bill (previous) (diff)

Changed 7 years ago by Mathevet julien

This one is extendable

comment:4 Changed 7 years ago by Mathevet julien

need to patch robot test to focus on current month.

Changed 7 years ago by Mathevet julien

Attachment: patchTestCalendar.diff added

comment:5 Changed 7 years ago by Mathevet julien

Well I didn't follow your advice. I think it's better to keep function to choose which datas are selected or disabled. So it could be extendeable.

comment:6 Changed 7 years ago by bill

What's not extensible about a _setSelectedDate() method?

comment:7 Changed 7 years ago by Mathevet julien

Oh... I spoke about _isSelectedDate.. In patch there is a methode selectDate too

Changed 7 years ago by Mathevet julien

Changed 7 years ago by Mathevet julien

Changed 7 years ago by Mathevet julien

comment:8 Changed 7 years ago by Mathevet julien

Thank's for email. I set your solution. sorry in last patch there are some console.log. Only thing is that we select and deselect nodes on each focus event. But it's better than svn version.

Changed 7 years ago by Mathevet julien

extract markSelected from focus

comment:9 Changed 7 years ago by Mathevet julien

I also remove setFocus on buildrendering because set("value") is call in _WidgetBase on build

comment:10 Changed 7 years ago by bill

Milestone: 1.8
Owner: set to bill
Status: newassigned

Changed 7 years ago by bill

updated patch, Calendar_a11y.patch passes regression w/out any changes, but I wonder if it's worth it to have a _markSelectedDates() method rather than leaving that logic in _populateGrid()

comment:11 Changed 7 years ago by bill

@moogle - We talked over email but I haven't heard from you in a while. I'm going to check in the patchUpdateGridOnChange9.patch I attached above, which is a modified version of your patch.

Like I said, I'm wondering if it's worth it to have an optimized code path for adjusting the selection, rather than just redrawing the grid when the selection changes. I suppose one advantage is that in a subclass, if a large range of dates is selected, at least we only need to scan that list of selected dates once, rather than 31 times.

comment:12 Changed 7 years ago by bill

Resolution: fixed
Status: assignedclosed

In [27696]:

Update Calendar to only redraw the grid when the month/year changes.

Replaced _isSelectedDate() with a _markSelectedDates() method, based on the idea that this.value should represent the currently selected date, or in subclasses, currently selected dates, so _isSelectedDate() is a strange way to know what dates are selected.

One small behavior change: when the DateTextBox contains an out-or-range value and the Calendar is opened, it doesn't show that value as selected.

Based on patch from Julien Mathevet (CLA), thanks!

Fixes #14407 !strict.

Note: See TracTickets for help on using tickets.