Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#12929 closed enhancement (fixed)

Even lighter charting widgets for mobile

Reported by: cjolif Owned by: cjolif
Priority: high Milestone: 1.8
Component: Charting Version: 1.6.1
Keywords: Cc: cjolif, Patrick Ruzand
Blocked By: Blocking:

Description

In 1.6 and 1.7 we made significant efforts to reduce the charting dependencies by removing un-needed ones (like Tooltip only used if available, not all plot types being required...). However we could go even further and have widget extending _WidgetBase instead of _Widget. However be careful because for desktop users this might remove some interesting features like focus management.

Maybe we could have two widgets one for mobile extending _WidgetBase and the existing one extending _Widget sharing of course the same code.

Attachments (1)

12929.patch (75.5 KB) - added by cjolif 8 years ago.
draft patch

Download all attachments as: .zip

Change History (15)

comment:1 Changed 8 years ago by Eugene Lazutkin

Adding special mobile widgets seems as the safer option for now.

comment:2 Changed 8 years ago by bill

FWIW, I looked at the _Widget uses in dojox/charting:

  • dojox.charting.widget.Chart
  • dojox.charting.widget.Legend

ISTM you could switch both of those to extend _WidgetBase without any issues (even on desktop).

You might also want to switch to the lightweight dojo/query.js rather than using dojo/_base/query.js.

BTW, about the Tooltip change (from #12208), I think rather than:

if(!dijit || !dijit.Tooltip){
	return;
}

you could do something like:

labelTooltip: function(elem, chart, label, truncatedLabel, font, elemType){
	require(["dijit/Tooltip"], function(){
		...
	});
}

That still avoids loading dijit/Tooltip unnecessarily, and avoids having to explain to the app developer when manually require dijit/Tooltip. (Although, maybe you want an explicit flag to say "display tooltips" or "don't display tooltips"?)

comment:3 in reply to:  2 Changed 8 years ago by Eugene Lazutkin

Replying to bill:

ISTM you could switch both of those to extend _WidgetBase without any issues (even on desktop).

If the change can be made in a backward-compatible manner, which doesn't break the existing functionality, we can implement it in the existing widget code.

But if we have a breaking change, even a small one, I strongly advocate to provide an alternative implementation.

I am sure cjolif will review his changes in this light and comes to a right decision, or consult others before doing anything drastic.

comment:4 Changed 8 years ago by cjolif

Cc: cjolif added

comment:5 Changed 8 years ago by cjolif

Cc: Patrick Ruzand added

Another thing that we could avoid to load in most cases: Theme is always importing dojox/Palette while it's only used in a few themes and use-cases.

Also Charting relies on gfx which might be improved with respect to that as well. For example gfx/utils always import dojo/_base/json even if you don't do serialization.

I will come up with a patch that tries to integrate Bill's idea and other ones while keeping compatibility.

Changed 8 years ago by cjolif

Attachment: 12929.patch added

draft patch

comment:6 Changed 8 years ago by cjolif

This patch does:

  • create SimpleTheme that do not import Palette and can be used in most cases. "old" Theme now inherits from SimpleTheme. Predefined themes that does not the full theme have been re-targeted to extend SimpleTheme.
  • remove _Widget dependency in favor of _WidgetBase. As Bill correctly supposed, my tests have shown no changes of behavior even on Desktop by using that base class. However obviously the base class is not the same so if someone was relying on the fact the base class of our widget is _Widget and not _WidgetBase then obviously there is an API incompatibility (even if the behavior is the same).
  • little changes here and there (like removing un-needed AMD dependencies).

Let me know if you have any comment. To me as the behavior is the same and I don't see much people relying on the exact base class of the Chart is _Widget not _WidgetBase I would be ok with that "incompatibility" (that I would list in the release notes). However let me know if you have a different feeling here.

comment:7 Changed 8 years ago by bill

The only breakage from going from _Widget --> _WidgetBase that I would expect would be people using the deprecated attr() method, rather than get() or set(). Personally I wouldn't worry about it, and just release note it.

(That's assuming that set()/get() have meaning for charts. Maybe they don't?)

Last edited 8 years ago by bill (previous) (diff)

comment:8 Changed 8 years ago by cjolif

Eugene, any comment on your side? If not I'll proceed with the commit next week.

comment:9 Changed 8 years ago by cjolif

Owner: changed from Eugene Lazutkin to cjolif
Status: newassigned

comment:10 Changed 8 years ago by cjolif

In [27760]:

refs #12929. Missing file.

comment:11 Changed 8 years ago by cjolif

In [27759]:

(The changeset message doesn't reference this ticket)

Having lighter charts especially for mobile (inherit _WidgetBase, have SimpleTheme?? vs Theme). Also fix some AMD dependencies to minimize them.

comment:12 Changed 8 years ago by cjolif

Resolution: fixed
Status: assignedclosed

comment:13 Changed 8 years ago by cjolif

In [28223]:

refs #12929. Fix small compatibility issue.

comment:14 Changed 7 years ago by cjolif

In [28710]:

remove reference to Palettte in SimpleTheme? that does not use it. refs #12929.

Note: See TracTickets for help on using tickets.