Problem/Motivation

The arguments field currently doesn't have a description, but it would be worth to have one, because

  • If arguments separated by / are supported, that should be explained
  • If multiple arguments (separated by +,)are allowed, the syntax is hard to remember. Should use the same description as views ui uses
  • If tokens are supported, explain that and enable the token browser

Steps to reproduce

Select a view from a views field and enter arguments being unsure about the correct syntax

Proposed resolution

See above.
Additionally maybe description for other fields may also make sense

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork viewsreference-3584133

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.

sapnil_biswas’s picture

sapnil_biswas commented
Assigned: Unassigned » sapnil_biswas

Would like to give it a try

sapnil_biswas opened merge request !102

sapnil_biswas’s picture

sapnil_biswas commented
Assigned: sapnil_biswas » Unassigned
Status: Active » Needs review
anybody’s picture

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

Nice work @sapnil_biswas - I left some final comments. This is very close to what I expected.

sapnil_biswas’s picture

sapnil_biswas commented

Thank you Sir @anybody for your feedbacks and appreciation. I will get the necessary fixes done and mark it for review once its all resolved

sapnil_biswas’s picture

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

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

Nice work @sapnil_biswas! I think just the token part might need to get improved a bit. Did you find any existing implementation in other contrib modules or core solving this?

The other parts seem perfectly well already, thank you! Token isn't that easy here indeed.

scott_euser’s picture

scott_euser commented

Thanks both! While you're at it, two comments from me:

  1. I think we normally avoid concatenating translated strings to keep it easier for translators, so probably two separate translations, one with and one without token support would make that easier?
  2. We don't need to test field description, that is already covered by Drupal Core, I don't think we are doing anything special here to doubt that coverage but we are now introducing token support, which would be good to cover

Thanks!

sapnil_biswas’s picture

sapnil_biswas commented
Status: Needs work » Needs review

I took out the extra token browser support and the test that was only for the description. Instead, I focused on adding a short description to the arguments field and used the existing Views UI translation for the contextual filter syntax.

Please let me know if you have a preferred direction for any further changes, as I would like to make sure that the scope stays in line with your expectations.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Good point! Indeed the (helpful!) token browser addition should go into a separate issue. Your code was not bad, would you like to open an issue for that this the code and a test?

I'll finally review this one, but I think it's good.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Reviewed & tested by the community

Just compared the texts to core and I think it's perfect.

The first one is a 1:1 match: https://git.drupalcode.org/project/drupal/-/blob/main/core/modules/views...
The second one can't be used in this context, but has been aligned well: https://git.drupalcode.org/project/drupal/-/blob/main/core/modules/views...

So in this case concatenating the translation strings makes sense to reuse the same string from core where possible (IMHO).

Great work @sapnil_biswas!

@scott_euser if you're also fine with the improvements, this is good to go!

scott_euser’s picture

scott_euser commented

Thanks! The only thing I am uncertain of is how this relates to #3558104: Add Contextual Filters Description on Arguments Field - is there anything in there that this is missing? If so, can we combine that into here (and I'll credit anyone who contributed there then)

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@scott_euser I think the text and general description here is much better and uses the core texts.
The other one then on top ads information about the contextual filters.

If I had to make the choice, I'd merge this one now and then rework the other one on top, especially because I think the other one is complex enough to need tests and we need to be careful about security there in the output. So I think the other one still NW.

But of course you decide.

scott_euser’s picture

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

Sounds good, thank you!

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.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thank YOU @scott_euser!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.