[WIP][Calendar] - Remove forced plug of CalendarNavigator and updated navigation control templates #680

Open
wants to merge 15 commits into
from

Projects

None yet

3 participants

@gerardkcohen
Contributor

The Calendar widget forces the plug of CalendarNavigator in the initializer, which breaks the purpose of a plugin being an instance-level enhancement/ augmentation. With this modification, I have added a navigatorPlugin attribute that developers can use to specify a navigator without having to unplug the forced CalendarNavigator. Of course, the default value is Y.Plugin.CalendarNavigator.

Along with this I have updated the CalendarNavigator prev/ next month controls to use buttons instead of anchor tags for better semantics and accessibility. CSS updated to support this.

Finally, I externalized the CalendarNavigator's aria-label strings using the Y.Intl utility by adding calendarnavigator.js and calendarnavigator_en.js to the lang directory.

Updates to the apidocs have been done, and I tested these changes on Win 7 with the following browsers: IE8, Chrome 26.0.1410.64, Firefox 20.0.1, Safari 5.1.7. Updates to unit tests not necessary.

@juandopazo juandopazo commented on the diff Apr 30, 2013
...endarnavigator/skins/night/calendarnavigator-skin.css
@@ -4,6 +4,9 @@
height: 14px;
background: transparent url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABgAAAAOCAYAAAA1+Nx+AAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAALZJREFUeNpi/P//PwORQBqIu6DsMiB+SpQukAUEMCcQVwPx5/8I8BkqxklIPyHDA4H44X/c4CFUDckWGADxgf/EgwNQPQQtEALiaf/JB9OgZmBYwAzERUD84T/l4APULGaYBR5AfPU/9QHITA8maGJiY6A+YENOpmw0CCI2bJEsCsRzKDB8DtSMgUum6DiKiIwWRUlOhhUVdUD8Dcngb1AxgkUFIwmFnRwQt0PZlUD8iBhNAAEGALbfq7SGiQoPAAAAAElFTkSuQmCC);
background-repeat: no-repeat;
+ border: none;
+ margin: 0;
@juandopazo
juandopazo Apr 30, 2013 Member

This is necessary because you changed the default tag to a button, isn't it?

@gerardkcohen
gerardkcohen Apr 30, 2013 Contributor

Correct

@juandopazo juandopazo commented on an outdated diff Apr 30, 2013
src/calendar/js/calendar.js
@@ -80,6 +79,12 @@ Y.Calendar = Y.extend(Calendar, Y.CalendarBase, {
var contentBox = this.get('contentBox'),
pane = contentBox.one("." + CAL_PANE);
+
+
+ this.after('navigatorPluginChange', function (e) {
+ e.prevVal && this.unplug(e.prevVal);
@juandopazo
juandopazo Apr 30, 2013 Member

This should go in a prototype function so it can be monkey-patched if necessary.

@juandopazo juandopazo and 1 other commented on an outdated diff Apr 30, 2013
src/calendar/js/calendar.js
+ },
+
+ /**
+ * The plugin to use for Calendar navigation controls
+ * allow dates later than this one to be set, and will reset any later date to
+ * this date. Should be `null` if no maximum date is needed.
+ *
+ * @attribute navigatorPlugin
+ * @type Function
+ * @default Y.Plugin.CalendarNavigator
+ */
+
+ navigatorPlugin: {
+ value: Y.Plugin.CalendarNavigator,
+ setter: function (val) {
+ if (Y.Lang.isFunction(val)) {
@juandopazo
juandopazo Apr 30, 2013 Member

This looks like a validator, not a setter. You could do validator: Y.Lang.isFunction

@gerardkcohen
gerardkcohen May 1, 2013 Contributor

You are right, I was originally doing more stuff in there that is now no longer necessary. Will simplify it by just using the validator.

@juandopazo
Member

I'm not a fan of the plugin-as-attribute approach. I think I'd rather see a more generic plugins: Plugin[] writeOnce attribute.

Just my 2¢.

@gerardkcohen
Contributor

@juandopazo Thank you for your comments. I too would rather use a more generic approach which is why I generated this pull request in the first place. I am struggling with getting your suggestion of plugins: Plugin[] to work by setting this in calendar.js since it is a non-attr and is only read from the calendar instance config. What am I missing?

@juandopazo
Member

I was thinking about something like this:

initializer: function () {
  var plugins = this.get('plugins');
  if (plugins) {
    this.plug(plugins);
  }
},
// ...
ATTRS: {
  plugins: {
    value: [Y.Plugin.CalendarNavigator],
    initOnly: true
  }
}

But it's just a suggestion. I'd rather hear from someone with more Calendar experience.

@gerardkcohen
Contributor

Ok, I completely misunderstood. I thought you meant use the plugins property that it is inherited from Base but you were just commenting on the name of the attr. Fair enough. The plugins property from Base is only read from the config that is passed to the constructor of the calendar instance. Because of that, I was thinking of this:

initializer : function (cfg) {
        if (!cfg.plugins) {
            cfg.plugins = this.get('plugins');
        }
    }

I wasn't sure how else to go around it since they are treated differently.

@juandopazo
Member

I totally forgot that Base did that by itself! I think it makes sense to try to use that instead of an extra attribute. I'm looking at how plugins works because it's not a regular attribute.

@juandopazo
Member

It seems that there is an undocumented _PLUG static property that lets you define plugins that are plugged by default. Subclasses can even define an _UNPLUG property that disables the plugging of certain plugins defined in _PLUG.

I don't know how stable this property is. If there isn't any plan for deprecating it, I'd use it for this case. Users could then either disable the CalendarNavigator plugin by subclassing or by modifying Y.Calendar._PLUG.

@gerardkcohen
Contributor

@juandopazo Thanks again for the info/ help, I will definitely look into it. However, how would you expect an average user designate which navigator plugin to use? Subclassing or modifying Y.Calendar._PLUG seems a little extreme, no?

@gerardkcohen
Contributor

I was thinking about this a little more, and setting the static _PLUG puts me back in the same place, in that now all instances of the calendar will have that (forced) plugin. Sure, I could also define the _UNPLUG but that just feels dirty, paying the cost of plugging and unplugging what was not needed in the first place. Also, my above snippet would clobber the navigator plugin if someone did something like plugins: [Y.Plugin.Resize]. I think the navigatorPlugin attribute I am proposing in the pull request allows a user to specify which navigator to use, and if not the default will be used as specified by the Calendar, or any extension of Calendar. I might change it to just navigator...

@juandopazo
Member

We discussed this pull request at yesterday's Roundtable. There wasn't much consensus about what to do about this pull request since currently there is no-one taking ownership of this module, but I think I have an idea about how to address this. The downside of setting the plugin as an attribute is that if you want to use another navigation plugin then you're loading the whole code for the default plugin anyway. This defeats the purpose of having a modularized library like YUI.

The basic idea of how the Calendar module is structured is consistent with other modules: have a core CalendarBase class on top of which to build flavors of calendars and an extra fully featured Calendar class. Normally instead of changing the navigator in Calendar you'd create your own flavor based on CalendarBase. That's how other components like DataTable and Autocomplete work.

So I'd like your input on a couple of questions. Have you considered using CalendarBase instead of Calendar? If so, have you encountered any issues? Are there features in Calendar that CalendarBase doesn't have and that you think would better belong in the base class?

@gerardkcohen
Contributor

@juandopazo Thank you for taking the time to look into this. I think it would be fair to say that Calendar needs to be looked at again, and my PR is too small of a step to fix the issues. I have tried using CalendarBase instead of Calendar but realized that there was a lot of useful things missing that Calendar provided and would have needed to duplicate, such as keyboard nav and highlighting of cells, etc. My PR simply exposes that in order to have different Navigators, you have to do some ugly things. Let me know what I can do.

@juandopazo
Member

that there was a lot of useful things missing that Calendar provided and would have needed to duplicate, such as keyboard nav and highlighting of cells

Yup.

My guess is that the best way to go forward would be to move the behavior that Calendar provides into one or more class extensions (ie: CalendarKeys) and then make Calendar be a mix of those extensions. Then you could make your own calendar choosing which of all those extensions you want. I'll look into it during the weekend.

@gerardkcohen
Contributor

Thanks Juan, however I would say almost all of Calendar is reusable and not specific to any one navigator. CalendarKeys would be a start.

@triptych
Contributor

@gerardkcohen is this pull request ready to be reviewed or is it still a WIP? (if so, please add WIP to the title).
If it's ready to be reviewed, it'll need some unit tests as well as doc + HISTORY.md additions as well.

@gerardkcohen
Contributor

I think it opened up some other issues/ discussions about the architecture and @juandopazo said he would look into it.

@juandopazo
Member

I'm sorry I never got back to you on this.

I actually tried to split Calendar into extensions but it turns out pretty much everything in Calendar is about keyboard navigation and selection. So I ended up with a CalendarSelection extension and a Calendar class that just mixes in that extension and plugs the navigator in:

/**
Create a calendar view to represent a single or multiple
month range of dates, rendered as a grid with date and
weekday labels.

@class Calendar
@extends CalendarBase
@uses CalendarSelection
@param config {Object} Configuration object (see Configuration attributes)
@constructor
**/
Y.Calendar = Y.Base.create('calendar', Y.CalendarBase, [Y.CalendarSelection], {
    initializer: function () {
        this.plug(Y.Plugin.CalendarNavigator);
    }
});

I have a calendar-split branch for this. Would you like to take a look? https://github.com/juandopazo/yui3/tree/calendar-split/src/calendar/js

@gerardkcohen
Contributor

@juandopazo Sorry for the very long delay. FYI, congrats on the hire, it took too long.

I checked your branch, and splitting the code as you have is a great way to go. However, it still does not address my original concern, that is automatically plugging in the CalendarNavigator. What I was hoping to do was to easily allow custom/additional navigators without having to plug/unplug.

Plugins should be instance level, no?

@juandopazo
Member

Yes, my point was that this way you can create your own version of Calendar instead of having the navigator included by default. We could then put this extension in a new module and invent a new Calendar class (name ideas welcome) in YUI that doesn't include the navigator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment