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

Adding an alternate layout parameter to custom fields #18571

Merged
merged 9 commits into from Jun 2, 2018

Conversation

Projects
None yet
9 participants
@Bakual
Contributor

Bakual commented Nov 14, 2017

Pull Request for Issue #18162 .

Summary of Changes

Adds a new parameter "Layout" to custom fields which allows to select a different field layout for the automatic display.

Testing Instructions

  • Create alternative layouts for a custom field. The original one is components/com_fields/layout/field/render.php, it can be overriden at component and template level, so for com_content in components/com_content/layout/field/, templates/your_template/html/layouts/com_content/field and templates/your_template/html/layouts/com_fields/field. The file isn't allowed to contain an underscore.
  • Create a custom field and change the layout for that field, assign some value to it
  • Check in frontend if that layout is used as expected.

Expected result

Layout is used as specified

Actual result

Not possible to choose a different layout

Documentation Changes Required

Since it is a new feature, it has to be documented.

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Nov 14, 2017

Contributor

One thing I noted is that when you choose an override from a template which isn't active, the field will not show. JLayouts only work from the active template.

Contributor

Bakual commented Nov 14, 2017

One thing I noted is that when you choose an override from a template which isn't active, the field will not show. JLayouts only work from the active template.

@Bakual Bakual added the New Feature label Nov 14, 2017

@AndySDH

This comment has been minimized.

Show comment
Hide comment
@AndySDH

AndySDH Nov 15, 2017

Hey there,
great job with doing this!

I'm testing it and it does work correctly :) There are a couple of issues to address though:

  • I noticed that you've put a "check" that hides the select input if Automatic Display is set to "No". However, you shouldn't do this, because this layout selection should (and indeed does) come into place also when you insert a field as part of a Field Group in the content with the content plugin, as described in #18162. So the select input field is always valid and doesn't need to be hidden away if Automatic Display is set to "No".

  • I had my layouts in templates/your_template/html/layouts/field (which worked fine for the Content Plugin) and they were not showing up here in the select, so I think this folder is missing from the list. Yes, I know, yet another folder to add... didn't realize there were so many. Not sure why both this and templates/your_template/html/layouts/com_fields/field are used...

  • Another thing, the "render" layout should probably not show up in the select list, as that equals to "Use Default". Unless by "Use Default" you mean the very default from the core which is not overriden? Because if you include an overridden render.php, then it should not be in the list. Maybe you could change the default option to "Use Default (render)"?

Also, a question, why is it that a filename cannot have an underscore? So far I've been using underscores for the filenames of my layouts manually used with the Content Plugin and they've been working normally. Not a big deal, just wondering.

Great job again!

AndySDH commented Nov 15, 2017

Hey there,
great job with doing this!

I'm testing it and it does work correctly :) There are a couple of issues to address though:

  • I noticed that you've put a "check" that hides the select input if Automatic Display is set to "No". However, you shouldn't do this, because this layout selection should (and indeed does) come into place also when you insert a field as part of a Field Group in the content with the content plugin, as described in #18162. So the select input field is always valid and doesn't need to be hidden away if Automatic Display is set to "No".

  • I had my layouts in templates/your_template/html/layouts/field (which worked fine for the Content Plugin) and they were not showing up here in the select, so I think this folder is missing from the list. Yes, I know, yet another folder to add... didn't realize there were so many. Not sure why both this and templates/your_template/html/layouts/com_fields/field are used...

  • Another thing, the "render" layout should probably not show up in the select list, as that equals to "Use Default". Unless by "Use Default" you mean the very default from the core which is not overriden? Because if you include an overridden render.php, then it should not be in the list. Maybe you could change the default option to "Use Default (render)"?

Also, a question, why is it that a filename cannot have an underscore? So far I've been using underscores for the filenames of my layouts manually used with the Content Plugin and they've been working normally. Not a big deal, just wondering.

Great job again!

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Nov 15, 2017

@AndySDH please mark your Test as successfully:

  • open Issue Tracker
  • Login with your github-Account
  • Click on blue "Test this"-Button above Authors-Picture
  • mark your Test as successfully
  • hit "submit test result"

franz-wohlkoenig commented Nov 15, 2017

@AndySDH please mark your Test as successfully:

  • open Issue Tracker
  • Login with your github-Account
  • Click on blue "Test this"-Button above Authors-Picture
  • mark your Test as successfully
  • hit "submit test result"
@AndySDH

This comment has been minimized.

Show comment
Hide comment
@AndySDH

AndySDH Nov 15, 2017

@franz-wohlkoenig Sure, thanks for the guidelines! I'll be waiting for @Bakual about the fixes above and then will do that :)

AndySDH commented Nov 15, 2017

@franz-wohlkoenig Sure, thanks for the guidelines! I'll be waiting for @Bakual about the fixes above and then will do that :)

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Nov 15, 2017

Contributor

because this layout selection should (and does come) into place also when you insert a field as part of a Field Group in the content with the content plugin

Ah true, didn't think of that. My though was to prevent misunderstanding when someones wants to insert a single field manually. I think then the layout doesn't apply (but haven't tested) and that could be confusing.

templates/your_template/html/layouts/field

That is then a lookup folder from JLayouts overriding mechanism itself. Looks indeed like I have to add that one as well. The com_fields one is because that is where the default layout actually is stored.

Another thing, the "render" layout should probably not show up in the select list

Yes, it should not. The "Use Default" should cover that. That was my plan. Do you have an override for that in your template (I think I didn't test that case)? Or why did it show up?

Also, a question, why is it that a filename cannot have an underscore?

Not really a technical reason. I based the formfield on the component layout one. In view layouts, we use underscore in layout files which should not appear in the select list. That allows to have "children" layouts. If you for example look at the article view, you see the default.php and a default_link.php file. The latter is a child layout of the former and doesn't appear in the article layout list.
I thought it makes sense to keep that naming convention to allow "hiding" certain layout files.

Contributor

Bakual commented Nov 15, 2017

because this layout selection should (and does come) into place also when you insert a field as part of a Field Group in the content with the content plugin

Ah true, didn't think of that. My though was to prevent misunderstanding when someones wants to insert a single field manually. I think then the layout doesn't apply (but haven't tested) and that could be confusing.

templates/your_template/html/layouts/field

That is then a lookup folder from JLayouts overriding mechanism itself. Looks indeed like I have to add that one as well. The com_fields one is because that is where the default layout actually is stored.

Another thing, the "render" layout should probably not show up in the select list

Yes, it should not. The "Use Default" should cover that. That was my plan. Do you have an override for that in your template (I think I didn't test that case)? Or why did it show up?

Also, a question, why is it that a filename cannot have an underscore?

Not really a technical reason. I based the formfield on the component layout one. In view layouts, we use underscore in layout files which should not appear in the select list. That allows to have "children" layouts. If you for example look at the article view, you see the default.php and a default_link.php file. The latter is a child layout of the former and doesn't appear in the article layout list.
I thought it makes sense to keep that naming convention to allow "hiding" certain layout files.

@AndySDH

This comment has been minimized.

Show comment
Hide comment
@AndySDH

AndySDH Nov 15, 2017

Ah true, didn't think of that. My though was to prevent misunderstanding when someones wants to insert a single field manually. I think then the layout doesn't apply (but haven't tested) and that could be confusing.

You have a point. Yeah, the layout indeed does not apply when you insert a single field manually. Maybe for clarity we could rename the option to "Automatic Layout", and in the option description, change it to "Choose an alternate layout to be used with Automatic Display or as part of a Field Group."

Unless... we want to actually make it work also for when inserting a single field manually? (if that is even possible). This way, the option could act as "Default/Base Layout", which is always used for that field unless otherwise overridden in the single field via content plugin.

That is then a lookup folder from JLayouts overriding mechanism itself. Looks indeed like I have to add that one as well. The com_fields one is because that is where the default layout actually is stored.

Yeah, figured so... One more folder to add, damn! Haha.

Yes, it should not. The "Use Default" should cover that. That was my plan. Do you have an override for that in your template (I think I didn't test that case)? Or why did it show up?

Yeah, I have an override for render.php now in templates/your_template/html/layouts/com_fields/field and it's showing up in the list, it looks like.

In view layouts, we use underscore in layout files which should not appear in the select list. I thought it makes sense to keep that naming convention to allow "hiding" certain layout files.

Gotcha, that makes sense to me too. Good idea/decision 👍

AndySDH commented Nov 15, 2017

Ah true, didn't think of that. My though was to prevent misunderstanding when someones wants to insert a single field manually. I think then the layout doesn't apply (but haven't tested) and that could be confusing.

You have a point. Yeah, the layout indeed does not apply when you insert a single field manually. Maybe for clarity we could rename the option to "Automatic Layout", and in the option description, change it to "Choose an alternate layout to be used with Automatic Display or as part of a Field Group."

Unless... we want to actually make it work also for when inserting a single field manually? (if that is even possible). This way, the option could act as "Default/Base Layout", which is always used for that field unless otherwise overridden in the single field via content plugin.

That is then a lookup folder from JLayouts overriding mechanism itself. Looks indeed like I have to add that one as well. The com_fields one is because that is where the default layout actually is stored.

Yeah, figured so... One more folder to add, damn! Haha.

Yes, it should not. The "Use Default" should cover that. That was my plan. Do you have an override for that in your template (I think I didn't test that case)? Or why did it show up?

Yeah, I have an override for render.php now in templates/your_template/html/layouts/com_fields/field and it's showing up in the list, it looks like.

In view layouts, we use underscore in layout files which should not appear in the select list. I thought it makes sense to keep that naming convention to allow "hiding" certain layout files.

Gotcha, that makes sense to me too. Good idea/decision 👍

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Nov 15, 2017

Contributor

Yeah, the layout indeed does not apply when you insert a single field manually.

I took another approach. The layout now applies also when you insert a single field 😄
You can still manually choose a different layout when inserting the field manually. You can even use the default one if you use "render" as layout option.

One more folder to add

Done.

Yeah, I have an override for render.php now in templates/your_template/html/layouts/com_fields/field and it's showing up in the list, it looks like.

Took care of them as well now.

Contributor

Bakual commented Nov 15, 2017

Yeah, the layout indeed does not apply when you insert a single field manually.

I took another approach. The layout now applies also when you insert a single field 😄
You can still manually choose a different layout when inserting the field manually. You can even use the default one if you use "render" as layout option.

One more folder to add

Done.

Yeah, I have an override for render.php now in templates/your_template/html/layouts/com_fields/field and it's showing up in the list, it looks like.

Took care of them as well now.

@AndySDH

This comment has been minimized.

Show comment
Hide comment
@AndySDH

AndySDH Nov 15, 2017

@Bakual Fantastic! Saw all your latest commits, perfect stuff. Everything looks thorughly taken care of. I will test this all once again with the latest changes and officially mark as successful.

AndySDH commented Nov 15, 2017

@Bakual Fantastic! Saw all your latest commits, perfect stuff. Everything looks thorughly taken care of. I will test this all once again with the latest changes and officially mark as successful.

@AndySDH

This comment has been minimized.

Show comment
Hide comment
@AndySDH

AndySDH Nov 15, 2017

I have tested this item successfully on 2d7e71e


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18571.

AndySDH commented Nov 15, 2017

I have tested this item successfully on 2d7e71e


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18571.

@AndySDH

This comment has been minimized.

Show comment
Hide comment
@AndySDH

AndySDH Nov 23, 2017

Can others test this? :)

AndySDH commented Nov 23, 2017

Can others test this? :)

@Quy

This comment has been minimized.

Show comment
Hide comment
@Quy

Quy Nov 23, 2017

Contributor

I have tested this item successfully on ea054e2


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18571.

Contributor

Quy commented Nov 23, 2017

I have tested this item successfully on ea054e2


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18571.

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Nov 23, 2017

@AndySDH can you please retest?

franz-wohlkoenig commented Nov 23, 2017

@AndySDH can you please retest?

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Nov 23, 2017

Ready to Commit after two successful tests.

franz-wohlkoenig commented Nov 23, 2017

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC label Nov 23, 2017

@laoneo

This comment has been minimized.

Show comment
Hide comment
@laoneo

laoneo Nov 23, 2017

Member

Sorry for being late to the party, but com_contact needs to be adjusted too https://github.com/joomla/joomla-cms/blob/staging/components/com_contact/layouts/fields/render.php#L70.

Member

laoneo commented Nov 23, 2017

Sorry for being late to the party, but com_contact needs to be adjusted too https://github.com/joomla/joomla-cms/blob/staging/components/com_contact/layouts/fields/render.php#L70.

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Nov 23, 2017

Contributor

Thanks for the reminder @laoneo !

Contributor

Bakual commented Nov 23, 2017

Thanks for the reminder @laoneo !

@AndySDH

This comment has been minimized.

Show comment
Hide comment
@AndySDH

AndySDH Nov 23, 2017

I have tested this item successfully on 8ba9ff9


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18571.

AndySDH commented Nov 23, 2017

I have tested this item successfully on 8ba9ff9


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18571.

@Quy

This comment has been minimized.

Show comment
Hide comment
@Quy

Quy Nov 24, 2017

Contributor

I have tested this item successfully on 8ba9ff9


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18571.

Contributor

Quy commented Nov 24, 2017

I have tested this item successfully on 8ba9ff9


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18571.

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 May 4, 2018

Contributor

@Bakual when you get a minute can you please update this pr and fix the merge conflicts?

Contributor

zero-24 commented May 4, 2018

@Bakual when you get a minute can you please update this pr and fix the merge conflicts?

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual May 5, 2018

Contributor

@zero-24 Rebased and fixed the conflicts

Contributor

Bakual commented May 5, 2018

@zero-24 Rebased and fixed the conflicts

@mbabker mbabker modified the milestones: Joomla 3.10.0, Joomla 3.9.0 Jun 2, 2018

@mbabker mbabker changed the base branch from staging to 3.9-dev Jun 2, 2018

@mbabker mbabker added PR-3.9-dev and removed PR-staging labels Jun 2, 2018

@mbabker mbabker merged commit 1fa4f9c into joomla:3.9-dev Jun 2, 2018

4 checks passed

Hound No violations found. Woof!
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joomla-cms-bot joomla-cms-bot removed the RTC label Jun 2, 2018

@Bakual Bakual deleted the Bakual:AlternateLayoutField branch Jun 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment