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

[Grid] Grid options #5247

Merged
merged 2 commits into from
Jun 16, 2016
Merged

[Grid] Grid options #5247

merged 2 commits into from
Jun 16, 2016

Conversation

dantleech
Copy link
Contributor

@dantleech dantleech commented Jun 14, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? no
Related tickets #5244
License MIT

This PR (will) integrate the OptionsResolver with the Grid component FieldTypeInterface and FilterInterface classes.

TODO:

  • FieldTypes
  • Filters Filters already have form types

@dantleech dantleech changed the title Grid options [Grid] Grid options Jun 14, 2016
$fieldType = $this->fieldsRegistry->get($field->getType());
$resolver = new OptionsResolver();
$fieldType->configureOptions($resolver);
$options = $resolver->resolve($field->getOptions());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think eventually both the retrieval of the field type service and the resolution of the options could be done at a higher level, and so the method signature for the rednerer could be something like:

public function renderField(GridView $gridView, Field $field, FieldTypeInterface $type, array $options, $data)

But nothing to do with this PR.

@pjedrzejewski pjedrzejewski added Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). DX Issues and PRs aimed at improving Developer eXperience. labels Jun 14, 2016
@dantleech
Copy link
Contributor Author

@pjedrzejewski the Filters have form types, which already have options. So, unless we separate the form options from the filter options then I guess it makes no sense to add additional option configuration (?).

@pjedrzejewski pjedrzejewski merged commit a7c2d21 into Sylius:master Jun 16, 2016
@pjedrzejewski
Copy link
Member

Good stuff, thanks Dan! Yes, in previous implementation filter forms had to have the same option configuration, even if they did not use these options which sucked, but I think I'd be fine with that, because it is better than having no control over and no default options. WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX Issues and PRs aimed at improving Developer eXperience. Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants