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

U4-5520: Localize date/time picker #504

Merged
merged 8 commits into from Oct 24, 2014

Conversation

@arknu
Copy link
Contributor

commented Oct 4, 2014

This updates the bootstrap-datetimepicker to a version that uses Moment.js for localization (http://eonasdan.github.io/bootstrap-datetimepicker/) and makes the necessary changes for that to work. With this, the datepicker is now fully localized.

@arknu

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2014

By the way, I left the seconds selector visible, as it was that way previously, but do we really need the user to be able to select seconds? I have certainly never encountered a need for that.

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Oct 6, 2014

We'll look into this for 7.3, too late for 7.2 now.

Also the time picker is used for when you use a Date/Time picker Property Editor, so yes it is required

@arknu

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2014

@shadem I was not referring to the timepicker as a whole - of course that is needed - but rather whether it should show hour, minute and seconds in the time picker or just hours and minutes. I think hours and minutes would be enough, as I have never had a use case for the timepicker including seconds.

@nul800sebastiaan

This comment has been minimized.

Copy link
Member

commented Oct 7, 2014

@arknu Some people actually need the seconds :) guess we could make it configurable but that seems a bit overkill.

@arknu

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2014

@nul800sebastiaan Yes, that would definitely be overkill. No reason to change anything, then.

@arknu arknu force-pushed the arknu:datetimepicker-locale branch from 8781c02 to 4913be8 Oct 14, 2014
@Shazwazza

This comment has been minimized.

Copy link
Member

commented Oct 16, 2014

I tried pulling this in but i have to revert it. We end up with some strange stuff when setting the time like: http://screencast.com/t/9m2DhScba

Before I don't think there was an AM/PM toggle and you can see when you change the time that the date format gets all mucked up. If you can fix these things and create a new PR that would be great.

@Shazwazza Shazwazza closed this Oct 16, 2014
@Shazwazza

This comment has been minimized.

Copy link
Member

commented Oct 16, 2014

Doh, sorry i didn't actually have to close this. You can keep this PR open, it just needs to be fixed.

@Shazwazza Shazwazza reopened this Oct 16, 2014
@arknu

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2014

@Shandem That is strange, that should have been fixed by my last commit. Will test again and fix ASAP.

@arknu

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2014

@Shandem I cannot reproduce your issue (have tested both clean install and upgrade). Are you sure you have picked up my last commit (4913be8)? That should have fixed it. There is a single changed .cs file as well for exactly this issue.
Otherwise, I'm going to need more repro steps.

@arknu arknu force-pushed the arknu:datetimepicker-locale branch from d1a8faf to caa4f3e Oct 16, 2014
@arknu

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2014

I have rebased to the latest 7.2.0 and tested again. I've removed the minified files per your changes, but I cannot reproduce the issue you mentioned.

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Oct 17, 2014

I've tried your latest code and i still have the issue... however, i the issue is limited to the date/time picker property editor, not the date/time picker used to select the publish at date/time. See: http://www.youtube.com/watch?v=7y7N5LlVPiQ

Strangely, the same property editor code is used for both those scenarios so not sure exactly what is going on there.

@arknu

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2014

Can you send me the site you use to reproduce? The only way I can reproduce this issue is by using the umbraco.dll from a normal 7.2.0 nightly without this patch. Then it behaves exactly as in your video. This is the only cause I can come up with. This would also explain why the publish/unpublish fields work as they use the default values in the .js file, not those from the Property Editor configuration.
The format strings for the pickers are defined in Umbraco.Web/PropertyEditors/DatePropertyEditor.cs and Umbraco.Web/PropertyEditors/DateTimePropertyEditor.cs.
The issues shown in the video are consistent with these strings not being updated.
Here are links to a compiled build as well as a demo site. Both work fine for me - perhaps you can test these?
Build: http://1drv.ms/1CuWLkt
Demo site: http://1drv.ms/1CuZ8nv
(user admin@example.com, password 1234)

@nul800sebastiaan Could you perhaps test this as well?

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Oct 24, 2014

Yay! It now works in my local build, pulling this in now.

Shazwazza added a commit that referenced this pull request Oct 24, 2014
U4-5520: Localize date/time picker
@Shazwazza Shazwazza merged commit a44123f into umbraco:7.2.0 Oct 24, 2014
@Shazwazza

This comment has been minimized.

Copy link
Member

commented Oct 27, 2014

Unfortunately it seems that the date pickers are no longer persisting any values. If you change a date and save the content item it appears changed, but then refresh the item and the data is not saved. Have created an issue: http://issues.umbraco.org/issue/U4-5690

Not 100% positive it's related to these changes but I'm assuming it is.

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Oct 27, 2014

Hrm, maybe not, i think it's actually related to one of these PRs:

#498
#491

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Oct 27, 2014

The problem is because the applyDate method is no longer firing based on the 'changeDate' event of the date picker.

This PR definitely seems to have changed all of that functionality.

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Oct 27, 2014

After looking at this a bit more, this PR seems to change quite a bit of this library and I'm not sure if it actually removes the functionality that was added in these PRs?
https://github.com/umbraco/Umbraco-CMS/pull/498/files
https://github.com/umbraco/Umbraco-CMS/pull/491/files

It definitely changes how the event system works and which events are fired which now breaks the date picker property editor.

I might have to revert this PR for 7.2 unless we are able to revert the event names and the data passed to the events, and ensure that the functionality in the other 2 PRs is maintained by today.

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Oct 27, 2014

Sorry, but i had to revert these changes: 87db6d6

It's pretty critical that there are no breaking changes in these libs and that the date pickers persist their values.

If you can open up another PR with all these mods but keep the event names and data the same, as well as keep the functionality done in the other PRs that would be great!

@arknu

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2014

Oops, that is indeed a pretty critical issue. Can't imagine how I missed that!
I'll have a look, but I guess that will be for 7.3, right?

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Oct 27, 2014

Maybe, we're gonna release 7.2 beta pretty quick so might be able to get it in for final release but we'll see. Otherwise def for 7.3. If you create the PR against 7.2 still that would be good (if you get time!)

@arknu

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2014

I really want to try to get this into 7.2 (the current picker is very annoying), so I'll work on it this week - and I shall test properly this time.
Regarding the event names. What would be your preferred solution - modifying the library directly to make it mimic the old datepicker (and thus make possible future updates a bit more complicated), or add an additional event with old name in the Angular controller (which I think ought to be possible)? The latter won't work, of course, if there are any external users that use the datepicker library directly, so maybe the first solution is the one at the moment.

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Oct 27, 2014

That's the issue, there might be other devs using the datetime picker lib directly. You'll have to modify the lib unfortunately but you can probably 'just' trigger the native new dp events and just add extra calls for the old legacy ones below them. Getting the correct data that is passed to the old events might be the trickier part.

@nul800sebastiaan

This comment has been minimized.

Copy link
Member

commented Oct 27, 2014

Sorry to rain on the parade but we're currently too close to releasing 7.2 beta and we can't update it after the beta any more. Sorry, wish it had worked properly the first time.

Also I'm not in favor of modifying libs! We know how that ends (I'm looking at you TinyMCE). I know this is a much smaller lib but still.

Of course if you need this functionality now, you can create a custom property editor and switch out the built-in datepicker with your own.

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Oct 27, 2014

The problem is that it's used for all dates not just datetime property
editors (i.e published at dates)

Also, we already have a modified version of an old lib that can't be found
on the Internet anymore so modified this new lib really isn't any worst
than what we have (actually it'd be much better)

But I guess 7.3 it is.

Sent from my phone

@arknu

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2014

I take it, then, that you're OK about making modifications to keep events the same in the new library (clearly marked, of course), but otherwise use the new library for 7.3.

Anyway, thanks for the suggestion about the a custom property editor. That is probably the way to go in the meantime, but as @Shandem said, that won't help for built-in fields, so I still think getting this fixed as soon as we can is important.

@arknu

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2014

A small status update: I have the issue basically fixed: old events are broadcast along with new ones. I have mapped dp.change > changeDate, dp.show > show, dp.hide > hide. The arguments for changeDate are using the same (incorrect) values from the previous datepicker. And of course saving actually works now.
I am running tests now, and will hopefully submit a new PR in a couple of hours, if all goes well.

@arknu

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2014

New PR #519 has been submitted with fixes for these issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.