plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
426 stars 574 forks source link

Dates widgets #1145

Closed nzambello closed 4 years ago

nzambello commented 4 years ago

Fixes #678 . Fixes #632 .

Different approch from #709 . That one from @cekk could be closed.

I added components for date picker and time picker because I've not found a component handling both of them I liked and which was documented, extendable and supported enough.

nzambello commented 4 years ago

In the next days I'll work on localization, accessibility and styles to make it pastanaga-ish. Any suggestion or help on styles would be appreciated, but I'm watching zeplin project.

nzambello commented 4 years ago

@sneridagh @tisto I don't know exactly who should I tag here, but my work on datetime widget is complete, it's accessibile, works on mobile, works with moment locales and it's pastanaga-ish. I added relative tests, but the cypress test isn't passing for a problem I'd like to discuss with you because I'm not finding out a way to resolve it.

While editing event dates it works well and dates are saved correctly but when you do the same saving a date on, for example, the effective date of any content, the date seems to be saved without timezone and from my position it become 1h earlier.

I'm investigating on the cause.

nzambello commented 4 years ago

Looking at requests, the string I'm submitting is like "2020-02-10T14:01:00.000Z" but what Plone give in GET is like "2020-02-10T14:01:00".

The problem here is that events dates work in the first way.

tisto commented 4 years ago

@nzambello this looks like a flickering Cypress test to me. The PR Travis build is passing and the Push one isn't. In those kinds of situations I usually just re-run Travis:

https://travis-ci.org/plone/volto/builds/648293245?utm_source=github_status&utm_medium=notification

I am happy to do a call in case that problem persists to see what I can do to help.

tisto commented 4 years ago

I will try to find time to check out the functionality asap. @sneridagh or @robgietema are the right people to do an in-depth React code review I guess.

tisto commented 4 years ago

@nzambello Travis is green now :)

tisto commented 4 years ago

@nzambello I can confirm the problem with the effective date. I enter 13:30 as the effective date, save and get 12:30 in the view.

tisto commented 4 years ago

@nzambello the strange thing is that setting the effective date works without the new datepickers:

1) Login as admin 2) Create a new Page 3) Enter effective date: 2020-02-20 12:30 4) Save page 5) Edit page again and check effective date -> Volto master: effective date is correct -> Volto dates-widget PR: effective date is 2020-02-20 11:30 (one hour off/less)

It would be interesting to compare the REST API calls from both and figure out what's different.

I checked the basic implementation from a user's perspective and it looks and feels great! Good work!!!

nzambello commented 4 years ago

@nzambello Travis is green now :)

Yeah maybe the cypress test isn't affected by the timezone issue, but I expected it would fail.

Now we should wait restapi team make these dates uniform on TZ format I guess.

sneridagh commented 4 years ago

@nzambello Just tested out and it's awesome

image

Just a small thing! Have you seen the small ? on the dates corner? It's slightly off. Could you fix it?

Also for discussion and that no ones get surprises, the build increased beyond the limit, because, of course, we added a nice thing, provided by an awesome library.

I'm +1 with this, the hell the bundle size. We will address it in 5.

/cc @robgietema @tisto @nzambello @pnicolli @giuliaghisini

If we all agree I would merge it right away.

PS: Also, we should fix the BundleSize watcher.

nzambello commented 4 years ago

Fixing it ASAP, I'll do it during the morning

sneridagh commented 4 years ago

@nzambello thanks!!

nzambello commented 4 years ago

@sneridagh here it is, now it has correct spacing, adapted from popup dimensions.

nzambello commented 4 years ago

@nzambello the strange thing is that setting the effective date works without the new datepickers:

1. Login as admin

2. Create a new Page

3. Enter effective date: 2020-02-20 12:30

4. Save page

5. Edit page again and check effective date
   -> Volto master: effective date is correct
   -> Volto dates-widget PR: effective date is 2020-02-20 11:30 (one hour off/less)

It would be interesting to compare the REST API calls from both and figure out what's different.

I checked the basic implementation from a user's perspective and it looks and feels great! Good work!!!

@sneridagh I don't know if I'd merge this PR due to this issue with dates formats from REST API. Without widget you can manipulate the string saving the correct value for effective date, although it's not so much usable for an end user.

cekk commented 4 years ago

I made some research about date formats. restapi returns dates with different formats:

"created": "2020-02-14T09:32:49+00:00",
"effective": "2020-02-14T10:32:53",
"modified": "2020-02-14T09:32:53+00:00",
"end": "2020-02-14T10:00:00+00:00",
"start": "2020-02-14T09:00:00+00:00",

As you can see, effective doesn't have timezone info. effective and modified are stored as DateTime objects and start and end are stored as datetime objects.

modified is manually set in serializer getting his value directly and this set correctly timezone offset.

effective is extracted with his serializer because it is a field in the schema (behavior) here . The serializer for that field, call a getter of the behavior, that makes a strange conversion of the date skipping timezone infos.

This is basically because effective has a different date format. I can fix it in several ways but if we don't want to touch p.a.dexterity's getter, probably the easiest way is to create a specific DX field serializer for DateTime objects.

@tisto @buchi any other ideas?

sneridagh commented 4 years ago

@cekk @buchi @tisto Didn't we decided that for storing all internal dates (the functional ones) we go all with UTC? I vaguely remember that, then make the frontend adapt it depending the local time of the browser.

@cekk @nzambello Can we make that assumption? So even if the date has to TZ, then assume it's UTC, then adjust the date displayed to the user depending on the current user TZ?

cekk commented 4 years ago

@sneridagh all dates exported by restapi are in UTC, except the ones that are stored in Plone as DateTime and are in some behaviors (like effective) that are converted "as-is" without take care of their timezone.

sneridagh commented 4 years ago

@cekk @buchi @tisto Didn't we decide that for storing all internal dates (the functional ones) we go all with UTC? I vaguely remember that, then make the frontend adapt it depending the local time of the browser.

@cekk @nzambello Can we make that assumption? So even if the date has to TZ, then assume it's UTC, then adjust the date displayed to the user depending on the current user TZ?

sneridagh commented 4 years ago

@nzambello @cekk @pnicolli I would like this to be in Volto 4 final. I want to cut it during next week. Do you think you could work on this to make it happen?

@buchi @tisto Didn't we decide that for storing all internal dates (the functional ones) we go all with UTC? I vaguely remember that, then make the frontend adapt it depending the local time of the browser.

Thanks!

tisto commented 4 years ago

@sneridagh @nzambello @cekk yes, we did decide to always store UTC, see:

https://github.com/plone/plone.app.event/issues/290 https://plonerestapi.readthedocs.io/en/latest/upgrade-guide.html#upgrading-to-plone-restapi-2-x https://kitconcept.com/blog/plone-beethoven-sprint-2018-report-day-3/ https://github.com/plone/plone.restapi/pull/493 https://docs.google.com/document/d/10y-KL53Pfk_2mZGvLJF8CPbw2H1x1ECMWST81w9hDU0/edit

nzambello commented 4 years ago

@sneridagh

@cekk @nzambello Can we make that assumption? So even if the date has to TZ, then assume it's UTC, then adjust the date displayed to the user depending on the current user TZ?

It's correct to think that. I also forced moment to handle dates as UTC.

nzambello commented 4 years ago

@sneridagh @tisto I found a way to assume that the date is UTC if no timezone is specified. I wasn't able to do it with momentjs but I resolved with a regex.

sneridagh commented 4 years ago

@nzambello I would merge it when #967 is merged into master, then release the new 5.0.0-alpha0