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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new Form WeekType #32061

Merged
merged 1 commit into from Nov 3, 2019
Merged

Add new Form WeekType #32061

merged 1 commit into from Nov 3, 2019

Conversation

@dFayet
Copy link

dFayet commented Jun 15, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #32029
License MIT
Doc PR coming soon

Update

After the first try, I've updated the field to have more options, and be more "straight".
The field acts like the DateTimeType or TimeType, various fields type (pure text, html5 type, select boxes), data validation, ....

For that I took the choice to update the DateTimeToStringTransformer and DateTimeToArrayTransformer to make them work with weeks format.

I was not sure if it was better to update them or create new ones, WDYT?

Before addind tests and docs, it would be nice to have your first thoughts/comments 馃槉

Do you need/want a small test repo?

@dFayet dFayet requested a review from xabbuh as a code owner Jun 15, 2019
@dFayet dFayet force-pushed the dFayet:feature/week_type branch from ee85412 to 4eb5bec Jun 15, 2019
@xabbuh xabbuh added the Form label Jun 16, 2019
@xabbuh xabbuh added this to the next milestone Jun 16, 2019
@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Jun 16, 2019

Note: input elements with type="week" are not supported in Internet Explorer or Firefox.

(not tested though)

Also im wondering if the underlying DateType can be used with a format Y-\WW[-w] (https://en.wikipedia.org/wiki/ISO_week_date)

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Jun 19, 2019

I wonder if we need to support different input formats for this form type. Right now it only supports a string that is in the same format as the one supported by the HTML5 week input type. However, I could imagine that some developers would like to have a kind of structure like an array with a week and a year element.

@dFayet

This comment has been minimized.

Copy link
Author

dFayet commented Jun 19, 2019

@ro0NL DateType doesn't like date format without y M d, I doubt it can be used directly, I thing the WeekType will be independant, more like the TimeType.

@xabbuh I love the idea, it will behave like the TimeType, right?

@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Jun 19, 2019

@dFayet maybe compatibility with DateType is too far fetched, yet i tend to believe year-weekno-weekday is just as valid as year-month-day for a date. Thus we should be able to use DateTime as a model... yet, im not sure what PHP is doing at this point: https://3v4l.org/7BMDB

@stof

This comment has been minimized.

Copy link
Member

stof commented Jun 19, 2019

If we add a form for type=week, I think it would make sense to also add a validator to ensure that a string contains an ISO week. this is especially important given that some browsers will fallback to a text type (which is fine by default; projects could add some JS-based picker as fallback).

If we want to improve the fallback a bit, we could add a pattern validation (\d{4}-W\d{2}) on the field so that the text fallback has client-side validation too. But I'm not sure we should do it in core (it might interact badly with a JS-based picker added by projects).

Anyway, I think adding a field type rendering a <input type=week /> makes sense.

@dFayet dFayet force-pushed the dFayet:feature/week_type branch from 6e39ad7 to 45b92dd Jul 5, 2019
@dFayet dFayet changed the title Add new Form WeekType [WIP] Add new Form WeekType Jul 5, 2019
@dFayet dFayet force-pushed the dFayet:feature/week_type branch 2 times, most recently from b9472ff to c6ef137 Jul 5, 2019
@dFayet dFayet force-pushed the dFayet:feature/week_type branch from c6ef137 to 1f5c7c1 Jul 12, 2019
@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Jul 21, 2019

@dFayet Is this still work in progress or are you waiting for a review? I don't want to put pressure on you just ensure that you are not desperately waiting for a review. :)

@dFayet

This comment has been minimized.

Copy link
Author

dFayet commented Jul 21, 2019

Your're right to ask @xabbuh :D

The WIP tag might be too much as, beside the tests, and the questions added in the edit of this PR, I think this one is ready for some reviews.

The subject of the DataTransformers is my main hesitation, should I write the tests now, or wait some feedback?

@dFayet dFayet changed the title [WIP] Add new Form WeekType Add new Form WeekType Jul 21, 2019
@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Jul 22, 2019

@dFayet Looking at how the updated transformers would look like, it seems to me that having new transformers would be better. We may then also reconsider the idea that the norm data should be DateTime objects. IMO that's not really needed and we could use an array with two elements as norm data instead as well.

@dFayet dFayet force-pushed the dFayet:feature/week_type branch 9 times, most recently from 9b5cb92 to 5aa4ce3 Jul 30, 2019
@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Aug 30, 2019

@dFayet Thank you for working on this. I have left you a bunch of comments. But they are actually only about a few details. The overall picture looks quite good to me. 馃憤

@andreybolonin

This comment has been minimized.

Copy link

andreybolonin commented Oct 16, 2019

no chance to be merged at 4.4 ?

@xabbuh xabbuh force-pushed the dFayet:feature/week_type branch 4 times, most recently from dca5642 to fea3d59 Oct 18, 2019
@xabbuh xabbuh force-pushed the dFayet:feature/week_type branch 2 times, most recently from 21dcd3d to e375018 Oct 19, 2019
@xabbuh xabbuh modified the milestones: next, 4.4 Oct 28, 2019
@xabbuh xabbuh force-pushed the dFayet:feature/week_type branch from e375018 to c4a2f02 Nov 1, 2019
@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Nov 1, 2019

@dFayet I have incorporated the review feedback. This looks ready to me now. Thank you for your work on this. I have kept your commit so you will still get the full credit.

@xabbuh
xabbuh approved these changes Nov 1, 2019
@dFayet

This comment has been minimized.

Copy link
Author

dFayet commented Nov 1, 2019

Thank to you @xabbuh but let's not forget that this is at least a 50-50 between us (+1 for you for your quick reaction)

@fabpot
fabpot approved these changes Nov 3, 2019
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Nov 3, 2019

Thank you @dFayet.

@fabpot fabpot mentioned this pull request Nov 3, 2019
fabpot added a commit that referenced this pull request Nov 3, 2019
This PR was merged into the 4.4 branch.

Discussion
----------

Add new Form WeekType

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #32029
| License       | MIT
| Doc PR        | <!--symfony/symfony-docs#...--> coming soon

----
#### Update

After the first try, I've updated the field to have more options, and be more "straight".
The field acts like the `DateTimeType` or `TimeType`,  various fields type (pure text, html5 type, select boxes), data validation, ....

For that I took the choice to update the `DateTimeToStringTransformer` and `DateTimeToArrayTransformer` to make them work with weeks format.

I was not sure if it was better to update them or create new ones, WDYT?

Before addind tests and docs, it would be nice to have your first thoughts/comments 馃槉

Do you need/want a small test repo?

Commits
-------

c4a2f02 Add new Form WeekType
@fabpot fabpot merged commit c4a2f02 into symfony:4.4 Nov 3, 2019
0 of 3 checks passed
0 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
fabbot.io Some changes should be done to comply with our standards.
Details
@dFayet dFayet deleted the dFayet:feature/week_type branch Nov 3, 2019
@Hanmac

This comment has been minimized.

Copy link

Hanmac commented Nov 7, 2019

when looking at the input week page, would be a nice idea to have min and max for this too? or maybe as Validator?

the step parameter might be a bit more complicated

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Nov 7, 2019

@Hanmac That's already possible:

$builder->add('week', WeekType::class, [
    'attr' => [
        'min' => '2018-W18',
        'max' => '2018-W26',
    ],
]);
wouterj added a commit to symfony/symfony-docs that referenced this pull request Nov 9, 2019
This PR was merged into the 4.4 branch.

Discussion
----------

Add new WeekType Documentation

Documents new symfony/symfony#32061 feature
Fix #12583

Commits
-------

8cd2e65 Add WeekType Documentation
This was referenced Nov 12, 2019
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

10 participants
You can鈥檛 perform that action at this time.