Problem/Motivation

Entity extra field views should support all major view settings that are relevant to enable or override in the extra field.

The most important option is to render the views title IMHO: #3123635: Add option to render view title and add schema check tests

But also other settings which similar modules already offer, especially https://www.drupal.org/project/viewfield would be nice to have.

Reviewed related Viewfield options:
- Render empty view title when output is forced to build.

Use this META issue as parent for such single issues.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork entity_extra_field-3578499

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

anybody created an issue. See original summary.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Issue tags: +Novice

Might be a nice (even if not easy) novice task now that #3123635: Add option to render view title and add schema check tests is in and similar options already exist in https://www.drupal.org/project/viewfield and similar modules.

sapnil_biswas made their first commit to this issue’s fork.

sapnil_biswas opened merge request !56

sapnil_biswas closed merge request !56

sapnil_biswas opened merge request !57

sapnil_biswas’s picture

sapnil_biswas commented
Status: Active » Needs review

@anybody
Marking this issue as Needs Review.

The MR is ready and addresses the proposed changes.
Would really appreciate feedback and suggestions for improvement.

Thanks!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thanks @sapnil_biswas for adding this one further option.

There are several others missing, see #3443048: Allow the Views plugin to support defining items per page

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Needs work

PS: Please prefix the new option with view_.

MR LGTM otherwise. Thanks!

sapnil_biswas’s picture

sapnil_biswas commented
Status: Needs work » Needs review

@anybody
I’ve addressed the review feedback in 782bb0d by renaming the setting to `view_always_build_output` and updating the label/title text accordingly. The follow-up pipeline is now passing, so I’m moving this back to needs review.

I’m still learning, so if there’s anything else you’d like me to adjust, I’d really appreciate the guidance.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Needs work

Thanks @sapnil_biswas I left two comments. Please also add a test that ensures that this does not change anything if the override is left empty.

Please also look into https://www.drupal.org/project/viewfield which other settings they offer that should be added here. First please add them to the issue summary.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Title: [META] Add further options for Entity Extra Field "Views" » Add further options for Entity Extra Field "Views"

Removing the [META] as it looks like we're gonna solve this here.

sapnil_biswas’s picture

sapnil_biswas commented
Issue summary: View changes
Status: Needs work » Needs review

I reviewed Viewfield for similar formatter settings. Besides rendering the view title and always building output, a possible follow-up option here is support for rendering the empty view title when output is forced to build.

I also addressed the remaining review feedback:
- 3b51f5d clarifies that leaving the items-per-page field empty keeps the view setting.
- c1af1c7 renames the config schema label to "Override Items per page".
- 6ead558 adds a kernel test to ensure an empty items-per-page override does not change the selected view's pager behavior.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: Unassigned » thomas.frobieter

Thanks, we should bring them into the correct logical order like in views and put the title option first.
Also I'll have to take some time to look up which (other) settings make sense for views.

I thought about offset, but I think it could even become too much and things should probably be solved in the view instead. @thomas.frobieter anything you'd find very useful or you're missing?

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Assigned: thomas.frobieter » Unassigned

@thomas.frobieter anything you'd find very useful or you're missing?

@anybody I'm not sure; probably anything I can think of right now would be too complicated and—as you already mentioned—should be configured in the View itself.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Needs work

Thanks @thomas.frobieter then let's keep it like this and resolve the remaining points.

sapnil_biswas’s picture

sapnil_biswas commented
Status: Needs work » Needs review

Reordered Views Plugin Option
Please let me know if there a requirement for any furthered changes.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: Unassigned » grevil

@sapnil_biswas I left one further comment. Would be great, if you could try that and add a test if it works.

Thanks, @grevil will do the final review.

sapnil_biswas’s picture

sapnil_biswas commented

Hey @grevil, I just wanted to follow up on my MR, When you have time, could you please look it over? Please let me know if I need to make any changes.

sapnil_biswas’s picture

sapnil_biswas commented

@anybody @grevil Is there any requirement for any kind of follow-up or changes required for this?

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thanks @sapnil_biswas - @grevil will review this soon, sorry.

grevil’s picture

grevil commented
Assigned: grevil » Unassigned
Status: Needs review » Needs work
Issue tags: -Novice

Added a few comments.

sapnil_biswas’s picture

sapnil_biswas commented
Status: Needs work » Needs review

@grevil Have worked on the review comments, please have a look when you have time

grevil’s picture

grevil commented

Hey @sapnil_biswas.

Sorry this went under my radar. Will take a final look now!

grevil’s picture

grevil commented
Status: Needs review » Needs work

Adjusted the code.

There is no need for the complex items per page logic. Simply making the config nullable does the trick.

Could you adjust the test? We don't need a separate testing class and the submit tests are not needed IMO. Only test that the rendered output is correct.

sapnil_biswas’s picture

sapnil_biswas commented
Status: Needs work » Needs review
grevil’s picture

grevil commented
Assigned: Unassigned » grevil
Status: Needs review » Needs work

@sapnil_biswas, thank you! Sorry did not realise the "ExtraFieldViewsPluginTest2" was already part of entity_extra_field. What a weird naming... in this case let's keep it as is.

But no need for you to restore and adjust everything again. This one is on me, I'll revert the deletion and add some finishing touches. Thanks for working on this! :)

grevil’s picture

grevil commented
Assigned: grevil » Unassigned
Status: Needs work » Reviewed & tested by the community

All green, lets get this merged!

grevil’s picture

grevil commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.