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

Introduces NumberMinMaxFilterType and IntegerMinMaxFilterType #14320

Merged
merged 5 commits into from Jul 12, 2019

Conversation

@tomas862
Copy link
Member

commented Jun 21, 2019

Questions Answers
Branch? develop
Description? As for products list page I need two inputs besides each - one type is for filtering ids while another type is for numbers such as price.
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? partialy required for #14267
How to test? Within this PR this component will not appear anywhere. It will be used by others once it is merged so no manual testing is required

To do after release of this:

  • Document it!

This change is Reviewable

@rokaszygmantas

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

$minMaxForm = $this->createForm(MinMaxType::class, [], [
            'required' => true,
            'min_field' => IntegerType::class,
            'min_field_options' => [
                'help' => 'inner help message',
            ],
            'max_field' => NumberType::class,
        ]);

maybe we should introduce two min max types, one for integer and one for float values (if it's needed)? I think it's better compared to having one generic min max type and having to define types when building the form.

@tomas862

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

$minMaxForm = $this->createForm(MinMaxType::class, [], [
            'required' => true,
            'min_field' => IntegerType::class,
            'min_field_options' => [
                'help' => 'inner help message',
            ],
            'max_field' => NumberType::class,
        ]);

maybe we should introduce two min max types, one for integer and one for float values (if it's needed)? I think it's better compared to having one generic min max type and having to define types when building the form.

@rokaszygmantas we have more then just two types:

MoneyType
PercentType
RangeType

but Im thinking right now that you might be right here - I thought about this as well.

Maybe we could have :

IntegerMinMaxType
NumberMinMaxType
PercentMinMaxType
MoneyMinMaxType

and we also have native type RangeType which is <input type="range"> so it would be RangeMinMaxType which basically adds two sliders

anyway this is closely related to my second problem which is:
how to control layout in grid since same html does not fit anymore so well.

two options:

  1. Either we will have GridIntegerMinMaxType, GridNumberMinMaxType etc...
  2. Either I try to apply some css magic to fit these types in Grid so the same type can be rendered in form and in grid.

@matks matks added the migration label Jun 24, 2019

@tomas862

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

gonna apply it for grid use case only - when someone will need it they can extend the functionality.

tomas862 added some commits Jul 1, 2019

adds integer min max type instead of global min max type to be more s…
…pecific and fastly reusable - adds default most common used attributes

@tomas862 tomas862 marked this pull request as ready for review Jul 2, 2019

@tomas862 tomas862 requested a review from PrestaShop/prestashop-core-developers as a code owner Jul 2, 2019

@tomas862 tomas862 changed the title [POC] Introduces MinMax form type Introduces NumberMinMaxFilterType and IntegerMinMaxFilterType Jul 2, 2019

@tomas862

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

@matks this one is required for continue of product page listing development - review when you can :)

@matks

matks approved these changes Jul 9, 2019

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Review is and I restart Travis

@tomas862

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

need restart Travis again - too random error to be related with this PR :D

@matks matks added this to the 1.7.7.0 milestone Jul 12, 2019

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

LGTM

@matks matks merged commit faf13fb into PrestaShop:develop Jul 12, 2019

2 checks passed

PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

mbadrani added a commit to mbadrani/PrestaShop that referenced this pull request Jul 18, 2019

Merge pull request PrestaShop#14320 from tomas862/features/min-max-fo…
…rm-type

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