Opened 10 years ago

Closed 9 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 10 years ago.
patchUpdateGridOnChange.2.diff (925 bytes) - added by Mathevet julien 10 years ago.
better one
patchUpdateGridOnChange.3.diff (3.3 KB) - added by Mathevet julien 10 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 10 years ago.
This one is extendable
patchTestCalendar.diff (9.3 KB) - added by Mathevet julien 10 years ago.
patchUpdateGridOnChange.5.diff (6.1 KB) - added by Mathevet julien 10 years ago.
patchUpdateGridOnChange.6.diff (6.5 KB) - added by Mathevet julien 10 years ago.
patchUpdateGridOnChange.7.diff (5.9 KB) - added by Mathevet julien 10 years ago.
patchUpdateGridOnChange.8.diff (6.6 KB) - added by Mathevet julien 10 years ago.
extract markSelected from focus
patchUpdateGridOnChange9.patch (8.0 KB) - added by bill 9 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 10 years ago by Mathevet julien

Changed 10 years ago by Mathevet julien

better one

comment:1 Changed 10 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 10 years ago by Mathevet julien

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

comment:2 Changed 10 years ago by Mathevet julien

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

comment:3 Changed 10 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 10 years ago by bill (previous) (diff)

Changed 10 years ago by Mathevet julien

This one is extendable

comment:4 Changed 10 years ago by Mathevet julien

need to patch robot test to focus on current month.

Changed 10 years ago by Mathevet julien

Attachment: patchTestCalendar.diff added

comment:5 Changed 10 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 10 years ago by bill

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

comment:7 Changed 10 years ago by Mathevet julien

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

Changed 10 years ago by Mathevet julien

Changed 10 years ago by Mathevet julien

Changed 10 years ago by Mathevet julien

comment:8 Changed 10 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 10 years ago by Mathevet julien

extract markSelected from focus

comment:9 Changed 10 years ago by Mathevet julien

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

comment:10 Changed 10 years ago by bill

Milestone: 1.8
Owner: set to bill
Status: newassigned

Changed 9 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 9 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 9 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.