Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Datepicker using Globalize 1.x #1341

Merged
merged 15 commits into from Jul 28, 2015
Merged

Datepicker using Globalize 1.x #1341

merged 15 commits into from Jul 28, 2015

Conversation

@rxaviers
Copy link
Member

rxaviers commented Sep 4, 2014

💥 WIP...

The goal is to have Datepicker working with Globalize 1.x.

Depends on

  • #1371 fnagel:calendar-value-pr3;

Todo

  • Rebase on datepicker branch
  • Destroy translation data (weekHeader, prevText, nextText , datePickerRole, currentText). Create user options instead;
  • Move external/date.js into ui/ (see #1260 (comment));
  • Fix RTL LTR direction (available at cldr.main("layout/orientation/characterOrder"), values are left-to-right or right-to-left);
  • Get currentText: "Today" from locale.
  • Fix Use narrow day names (e.g., "ﺥ" EEEEE) if short (e.g., "ﺎﻠﺨﻤﻴﺳ" EEE) is too lengthy;
  • Update all demos accordingly. (currently, demos/datepicker/localization.html has been updated)
<option value="en">English</option>
<option value="es">Spanish (Espa&ntilde;ol)</option>
<option value="zh">Chinese Simplified (简体中国)</option>

This comment has been minimized.

Copy link
@rxaviers

rxaviers Sep 4, 2014

Author Member

I've reverted back some locales. It's important to have Chinese, Spanish and Arabic. Because, they are the three most spoken languages in the world after English.

Two questions:

  1. How to write Chinese Simplified in Chinese? I have translated that in Google Translate and got the above.
  2. Does anyone know how to convert UTF-8 into that numeric entities? Anyway, why using entities and not utf-8 as is?

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Sep 5, 2014

Member

I never liked that we attempted to translate the language names. Anyone browsing our site has to have some level of English comprehension and I'm sure they can recognize their language written in English if they can read our docs and demo descriptions.

@rxaviers

This comment has been minimized.

Copy link
Member Author

rxaviers commented Sep 4, 2014

Need to fix:

  • LTR vs. RTL. (available at cldr.main("layout/orientation/characterOrder"), values are left-to-right or right-to-left)
  • Use narrow day names (e.g., "ﺥ" EEEEE) instead of short (e.g., "ﺎﻠﺨﻤﻴﺳ" EEE).
master arabic
this branch arabic2

OBS: the month name above is different, but the new one seems more popular. So, it seems fine to me. See https://translate.google.com/#en/ar/September

@rxaviers

This comment has been minimized.

Copy link
Member Author

rxaviers commented Sep 5, 2014

Fixed "Use narrow day names (e.g., "ﺥ" EEEEE) if short (e.g., "ﺎﻠﺨﻤﻴﺳ" EEE) if too lengthy".

master arabic
this branch arabic2
cldr.main([ "dates/calendars/gregorian/days/format/short", weekdays[ day ] ]).length > 3 ?
cldr.main([ "dates/calendars/gregorian/days/format/narrow", weekdays[ day ] ]) :
cldr.main([ "dates/calendars/gregorian/days/format/short", weekdays[ day ] ]),
fullname: cldr.main([ "dates/calendars/gregorian/days/format/wide", weekdays[ day ] ])

This comment has been minimized.

Copy link
@rxaviers

rxaviers Sep 5, 2014

Author Member

We may favor using Globalize functions instead of accessing the data directly.

This comment has been minimized.

Copy link
@fnagel

fnagel Sep 5, 2014

Member

Depends on if we plan to make Globalize optional for Calendar.

This comment has been minimized.

Copy link
@rxaviers

rxaviers Sep 5, 2014

Author Member

It seems harder for Datepicker, because we use cldrjs anyway. But, yeap. It would be nice if we were able to make something here similar to Spinner.

@rxaviers

This comment has been minimized.

Copy link
Member Author

rxaviers commented Sep 5, 2014

About LTR vs. RTL, I'm looking at the master's implementation. It either inserts elements before or after depending on this value.

It would be a much more elegant solution if we could use CSS instead to control this flow, adding a proper .rtl modifier class.

@rxaviers

This comment has been minimized.

Copy link
Member Author

rxaviers commented Sep 5, 2014

How is RTL handled in other widgets?

@tjvantoll

This comment has been minimized.

Copy link
Member

tjvantoll commented Sep 5, 2014

@rxaviers Datepicker is the only widget with an RTL implementation, and you'll want to talk to @scottgonzalez about the how this should work. I don't remember the specifics, but I remember a conversation with him about problems with RTL in master.

@fnagel

This comment has been minimized.

Copy link
Member

fnagel commented Sep 5, 2014

@tjvantoll @rxaviers There are some notes within the Datepicker wiki page (search for isRTL).

@scottgonzalez

This comment has been minimized.

Copy link
Member

scottgonzalez commented Sep 5, 2014

RTL in master is just flat out wrong with changing DOM positions. Just let the browser do the right thing.

@tjvantoll

This comment has been minimized.

Copy link
Member

tjvantoll commented Sep 5, 2014

This diff is going to be really hard to see until we merge @fnagel's branch in. I think we need the calendar-value-pr branch moved from @fnagel's fork to the canonical jquery/jquery-ui repo. That way you could base this PR off that branch.

@rxaviers

This comment has been minimized.

Copy link
Member Author

rxaviers commented Sep 5, 2014

This diff is going to be really hard to see until we merge @fnagel's branch in

Absolutely. That's the idea. For now, I'm only raising issues and fixing other things.

rxaviers added a commit that referenced this pull request Sep 13, 2014
- Fix "I never liked that we attempted to translate the language
  #1341 (comment)
@rxaviers rxaviers mentioned this pull request Sep 16, 2014
};
});

return $.ui.calendarDate = _Date;

This comment has been minimized.

Copy link
@rxaviers

rxaviers Sep 17, 2014

Author Member

What name we should use to export _Date? Should we place this object inside ui/calendar.js, so we don't have to expose this class publicly? Note it's not used anywhere else besides calendar.

This comment has been minimized.

Copy link
@fnagel

fnagel Sep 19, 2014

Member

I like the idea of using this without including calendar, so I would prefer to have this as a standalone object.


$.widget( "ui.datepicker", {
// FIXME can we make this dynamic?
calendarOptions = [ "buttons", "disabled", "locale", "eachDay", "max", "min", "numberOfMonths", "showWeek" ];

This comment has been minimized.

Copy link
@rxaviers

rxaviers Sep 17, 2014

Author Member

Can we make this dynamic?

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Sep 17, 2014

Member

Yes, by simply putting it into the prototype.

This comment has been minimized.

Copy link
@fnagel

fnagel Sep 19, 2014

Member

Please give me a hint how to do this.

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Sep 19, 2014

Member

Just put it in the prototype, like version, options, and everything else that's in the prototype.

This comment has been minimized.

Copy link
@fnagel
setDay: function( day ) {
var date = this.dateObject;
// FIXME: Why not to use .setDate?
this.dateObject = new Date( date.getFullYear(), date.getMonth(), day, date.getHours(),
date.getMinutes(), date.getSeconds() );
return this;

This comment has been minimized.

Copy link
@rxaviers

rxaviers Sep 17, 2014

Author Member

Any reason we don't simply use Date#setDate() in here?

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Jan 20, 2015

Member

This drops the milliseconds, but I doubt that's intentional. Let's fix it.

This comment has been minimized.

Copy link
@fnagel

fnagel Jan 20, 2015

Member

Not sure if this was needed on order to prevent referencing of the new variable.

this.options.max = this.options.locale.parseYMD( this.options.max );
}
if ( $.type( this.options.min ) === "string" ) {
this.options.min = this.options.locale.parseYMD( this.options.min );
}

This comment has been minimized.

Copy link
@rxaviers

rxaviers Sep 17, 2014

Author Member

A better name for parseYMD?

fnagel referenced this pull request in fnagel/jquery-ui Sep 29, 2014
Add calendar widget by copying and renaming datepicker widget files.
Remove datepicker functionality, options and methods from Calendar.
Remove calendar functionality, options and methods from Datepicker.
Adjust tests due to split and changed specification. Remove duplicated
demo files and fix some demos. Simplify calendar generation, use CSS
instead of inline styles. Fix destroy method. Make use of uniqueId
method. Fix focus highlighting when month is changed. Add version
property. Add common unit tests. Fix input keyboard handling.
@fnagel

This comment has been minimized.

Copy link
Member

fnagel commented Sep 29, 2014

@rxaviers Please note there is a new PR for Calendar: #1352

@rxaviers rxaviers force-pushed the datepicker-globalize-1.x branch from fa52580 to 4e65e51 Oct 1, 2014
rxaviers added a commit that referenced this pull request Oct 1, 2014
- Fix "I never liked that we attempted to translate the language
  #1341 (comment)
@rxaviers

This comment has been minimized.

Copy link
Member Author

rxaviers commented Oct 1, 2014

Rebased on fnagel:calendar-value-pr2

@fnagel

This comment has been minimized.

Copy link
Member

fnagel commented Jan 19, 2015

@rxaviers #1371 has been merged!

@rxaviers

This comment has been minimized.

Copy link
Member Author

rxaviers commented Jan 19, 2015

🎉 🎉

@jzaefferer

This comment has been minimized.

Copy link
Member

jzaefferer commented Jan 20, 2015

This still has many commits from the other PR, does this need a rebase to get rid of them?

@jzaefferer

This comment has been minimized.

Copy link
Member

jzaefferer commented Jan 20, 2015

As for RTL: Scott is putting together a meeting to discuss a UI-wide solution. @rxaviers should be there as well and provide some input from the CLDR perspective (what RTL data is provided by CLDR?). Since that might still take a while, I suggest ignoring RTL here and addressing that later.

@fnagel

This comment has been minimized.

Copy link
Member

fnagel commented Jan 20, 2015

@jzaefferer Yes, this will need another rebase as we chaged a bunch of older commits in the merged branch.

- Make unit tests work
@fnagel

This comment has been minimized.

Copy link
Member

fnagel commented Apr 30, 2015

@rxaviers Some questions regarding this branch / the implementation of Globalize:

  • I noticed that the first two days are the same (Sunday or Monday depending on locale). Can you give me a hint where to search for the issue?
  • What needs to be done to update to Globalize 1.0.0 (its alpha 7 now)? I've tried (using bower, adding globalize number) but run into a cldr data error.
fnagel added 2 commits Apr 30, 2015
- Fix merge conflict in value method
- Fix common unit tests
- Fix calendar localization tests
- Fix rebase regression: labels update on refresh
- Add new pass through options
- lint fixes
- Make all demos work
- Fix calendar localization demo
- Fix merge errors
@fnagel

This comment has been minimized.

Copy link
Member

fnagel commented Apr 30, 2015

My current working branch: https://github.com/fnagel/jquery-ui/tree/datepicker-globalize

Not sure if I should force push this PR or open a new one with reference and copied description. Opinions? @rxaviers @scottgonzalez @jzaefferer

@scottgonzalez

This comment has been minimized.

Copy link
Member

scottgonzalez commented Apr 30, 2015

A force push seems fine for this one.

@rxaviers

This comment has been minimized.

Copy link
Member Author

rxaviers commented Apr 30, 2015

I noticed that the first two days are the same (Sunday or Monday depending on locale). Can you give me a hint where to search for the issue?

What do you mean?

@rxaviers

This comment has been minimized.

Copy link
Member Author

rxaviers commented Apr 30, 2015

What needs to be done to update to Globalize 1.0.0 (its alpha 7 now)? I've tried (using bower, adding globalize number) but run into a cldr data error.

I need a little more details. Anyway a good getting started is Globalize examples. https://github.com/jquery/globalize/tree/master/examples

@rxaviers

This comment has been minimized.

Copy link
Member Author

rxaviers commented Apr 30, 2015

About passing i18n options to datepicker (e.g., locale, pattern, etc), I suggest the below (we had a discussion about it in the past, but I don't find a summary about it; anyway...)

Datepicker shouldn't be tight coupled with Globalize. Instead, it should expect formatters and parsers (functions needed for formatting and parsering the calendar and input values) to be passed from user land. Datepicker could use Globalize to fill in the default ones (i.e., for en). Datepicker could even have a helper that uses Globalize to automatically fill them in for other locales as well. An initial work has been started at a8d5e80 (note _calendarDateOptions). But, I would consider removing _setLocale and any var globalize = new Globalize( locale ) reference from within datepicker and expect all formatters and parsers to come from options.

Having Datepicker loosely coupled with Globalize the way I described may sound more work now. But, it will dramatically improve its maintenance over time + it will help developers to integrate datepicker into their own i18n workflow easily.

@rxaviers

This comment has been minimized.

Copy link
Member Author

rxaviers commented Apr 30, 2015

Not sure if I should force push this PR

Feel free to force push to it assuming no work here is lost. :)

@scottgonzalez

This comment has been minimized.

Copy link
Member

scottgonzalez commented Apr 30, 2015

I don't recall coming to that conclusion. That's the approach we agreed on for spinner, but I have always expected datepicker to have a hard dependency on Globalize.

@fnagel

This comment has been minimized.

Copy link
Member

fnagel commented Apr 30, 2015

I noticed that the first two days are the same (Sunday or Monday depending on locale). Can you give me a hint where to search for the issue?

What do you mean?

first-day-twice

@rxaviers

This comment has been minimized.

Copy link
Member Author

rxaviers commented Apr 30, 2015

Well remembered @scottgonzalez. Obviously, having datepicker to have a hard dependency on Globalize is up to you. I also remember that you wanted datepicker to be distributed working for en as a default, which needs to be taken into account in this implementation.

- Fix wrong calculation of weekdays
@dwachss

This comment has been minimized.

Copy link

dwachss commented Apr 30, 2015

Thank you for rewriting Datepicker with the widget factory and based on CLDR.
Two questions about where this is going:

  1. Will there be support for calendar systems other than gregorian? CLDR includes the data for localization but not the algorithms. Will Datepicker be extensible that way?
  2. The initial comment 'Get currentText: "Today" from locale'. That's not in CLDR as far as I can tell (can you get that from Relative Date Names?). What about the "Done" button--or will you leave that out?
@fnagel

This comment has been minimized.

Copy link
Member

fnagel commented Apr 30, 2015

  1. Will there be support for calendar systems other than gregorian? CLDR includes the data for localization but not the algorithms. Will Datepicker be extensible that way?

Afaik yes.

What about the "Done" button--or will you leave that out?

There is a new option buttons like in dialog -- you will need to set the text strings yourself. See https://github.com/jquery/jquery-ui/blob/datepicker/demos/calendar/buttonbar.html

Make sure to take a look at the wiki page: http://wiki.jqueryui.com/w/page/12137778/Datepicker

@rxaviers

This comment has been minimized.

Copy link
Member Author

rxaviers commented Apr 30, 2015

  1. Will there be support for calendar systems other than gregorian? CLDR includes the data for localization but not the algorithms. Will Datepicker be extensible that way?

I can't answer for the widget. But, it will depend on Globalize, whose support for other calendars need globalizejs/globalize#320 to be implemented. Help appreciated.

  1. The initial comment 'Get currentText: "Today" from locale'. That's not in CLDR as far as I can tell (can you get that from Relative Date Names?)

It's is in CLDR (in dateFields.json).

@dwachss

This comment has been minimized.

Copy link

dwachss commented Apr 30, 2015

Thanks. I've been working on an alternative-calendar-system datepicker widget for the past few years ( https://github.com/dwachss/flexcal ) and I have a good handle on manipulating them. I'll see if I have time to work on Globalize.
Thanks also for the link to the CLDR data (it's very hard to find from the Unicode site).
Danny   

@rxaviers

This comment has been minimized.

Copy link
Member Author

rxaviers commented Apr 30, 2015

Thanks @dwachss. Looking forward to it.

@fnagel fnagel force-pushed the datepicker-globalize-1.x branch from a8d5e80 to 28bc45c May 13, 2015
@fnagel fnagel merged commit 28bc45c into datepicker Jul 28, 2015
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
jQuery Foundation CLA All authors have signed the CLA
Details
rxaviers added a commit that referenced this pull request Jul 28, 2015
- Fix "I never liked that we attempted to translate the language
  #1341 (comment)
@rxaviers

This comment has been minimized.

Copy link
Member Author

rxaviers commented Jul 29, 2015

Can this branch datepicker-globalize-1.x be deleted?

In the description, there are two remaining items. Are they been tracked somewhere else?

@fnagel

This comment has been minimized.

Copy link
Member

fnagel commented Jul 29, 2015

I closed this by accident. It's not merged to datepicker branch yet.

datepicker-globalize-1.x is still up to date with my latest changes!

@rxaviers

This comment has been minimized.

Copy link
Member Author

rxaviers commented Jul 29, 2015

Gotcha. Thanks

@fnagel

This comment has been minimized.

Copy link
Member

fnagel commented Jul 29, 2015

More info in the UI dev meeting later today.

@fnagel

This comment has been minimized.

Copy link
Member

fnagel commented Aug 2, 2015

This PR has been merged to datepicker branch. Remaining issue is RTL LTR support.

@scottgonzalez Datepicker branch is now up to date and waiting for your merging magic :-D

@scottgonzalez

This comment has been minimized.

Copy link
Member

scottgonzalez commented Aug 20, 2015

I finished merging master into datepicker this morning.

@fnagel

This comment has been minimized.

Copy link
Member

fnagel commented Aug 25, 2015

Looks good! Added a new PR based on this: #1590

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.