JFormFieldList shows the global values if present. #11911

Merged
merged 13 commits into from Nov 4, 2016

Conversation

Projects
None yet
@Bakual
Contributor

Bakual commented Sep 3, 2016

Pull Request for Issue #11855.

Summary of Changes

The list formfield (the one responsible for dropdown selects in forms) will now show the global setting if available.
The field tries to load the component params and checks if the fieldname exists as param. If it does it will show the value.

Testing Instructions

Check the article settings and see that the global values are now shown behind within the select.
Check if there are any sideeffects in unexpected locations (mainly other forms).

Documentation Changes Required

None.
JFormFieldList now supports useglobal as an attribute. It will add the option "Use Global" and shows the value if found.

I honestly don't know if that should be included in core or not. I just saw the request and thought I see if I can do it. It feels a bit hacky to me to fiddle with extension params in a library class. Maybe someone even has a better idea.

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Sep 3, 2016

Contributor

IMHO always showing the global value is a BIG usability improvement!
with this we don't need to guess what is the global value, we have it right there.

Contributor

andrepereiradasilva commented Sep 3, 2016

IMHO always showing the global value is a BIG usability improvement!
with this we don't need to guess what is the global value, we have it right there.

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Sep 3, 2016

Contributor

this PR adds like this
image

I think it would be much more clean something like this
image

Also when null shouldn't it show the default value?

Contributor

andrepereiradasilva commented Sep 3, 2016

this PR adds like this
image

I think it would be much more clean something like this
image

Also when null shouldn't it show the default value?

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Sep 3, 2016

Contributor

I think it would be much more clean something like this

Showing the global value in the option would be an idea. However it could be tricky since you don't really know which of the options is "Use Global". Probably not every empty value will have the option "Use Global".

Also when null shouldn't it show the default value?

The default value of the global config file? Good luck writing the code for that 😄
If it's null, the param isn't saved yet or doesn't exist in global settings. So go ahead and save your options, that should fix it. If there is still no global setting shown and there is a "Use Global" option, congratulations: You found a bug in the extension 😄

Contributor

Bakual commented Sep 3, 2016

I think it would be much more clean something like this

Showing the global value in the option would be an idea. However it could be tricky since you don't really know which of the options is "Use Global". Probably not every empty value will have the option "Use Global".

Also when null shouldn't it show the default value?

The default value of the global config file? Good luck writing the code for that 😄
If it's null, the param isn't saved yet or doesn't exist in global settings. So go ahead and save your options, that should fix it. If there is still no global setting shown and there is a "Use Global" option, congratulations: You found a bug in the extension 😄

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Sep 3, 2016

Contributor

@Bakual travis is failing here? Some could not connect to database issues? I'm not sure what about the installation?

Contributor

zero-24 commented Sep 3, 2016

@Bakual travis is failing here? Some could not connect to database issues? I'm not sure what about the installation?

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Sep 3, 2016

Contributor

The default value of the global config file? Good luck writing the code for that 😄

No, i mean the default value of the param in the form xml.

If it's null, the param isn't saved yet or doesn't exist in global settings. So go ahead and save your options, that should fix it. If there is still no global setting shown and there is a "Use Global" option, congratulations: You found a bug in the extension 😄

So this could be used as a bug finder lol

Contributor

andrepereiradasilva commented Sep 3, 2016

The default value of the global config file? Good luck writing the code for that 😄

No, i mean the default value of the param in the form xml.

If it's null, the param isn't saved yet or doesn't exist in global settings. So go ahead and save your options, that should fix it. If there is still no global setting shown and there is a "Use Global" option, congratulations: You found a bug in the extension 😄

So this could be used as a bug finder lol

@dgrammatiko

This comment has been minimized.

Show comment
Hide comment
@dgrammatiko

dgrammatiko Sep 3, 2016

Member

@Bakual I think you might have to rebase this one to 3.7 as the field is now using JLayouts

Member

dgrammatiko commented Sep 3, 2016

@Bakual I think you might have to rebase this one to 3.7 as the field is now using JLayouts

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Sep 3, 2016

Contributor

@zero-24 I guess because the listfield now needs JInput and the extension parameters to calculate the global setting. Basically it shows the architectural flaw in this approach as it adds dependencies to that class which isn't the best thing to do.

No, i mean the default value of the param in the form xml.

Ideally, that should be the same, true. But I would still not use it. The user should save the options anyway.

So this could be used as a bug finder lol

Exactly 👍

On a sidenote, I already found an issue with this approach as it doesn't work for menu item options.

Contributor

Bakual commented Sep 3, 2016

@zero-24 I guess because the listfield now needs JInput and the extension parameters to calculate the global setting. Basically it shows the architectural flaw in this approach as it adds dependencies to that class which isn't the best thing to do.

No, i mean the default value of the param in the form xml.

Ideally, that should be the same, true. But I would still not use it. The user should save the options anyway.

So this could be used as a bug finder lol

Exactly 👍

On a sidenote, I already found an issue with this approach as it doesn't work for menu item options.

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Sep 3, 2016

Contributor

Love the idea but it doesnt seem to read everything

screen shot 2016-09-03 at 14 49 45


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

Contributor

brianteeman commented Sep 3, 2016

Love the idea but it doesnt seem to read everything

screen shot 2016-09-03 at 14 49 45


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

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Sep 3, 2016

Contributor

Ideally, that should be the same, true. But I would still not use it. The user should save the options anyway.

We all know, not all users, if any user, will go to all components and save those config params.

Contributor

andrepereiradasilva commented Sep 3, 2016

Ideally, that should be the same, true. But I would still not use it. The user should save the options anyway.

We all know, not all users, if any user, will go to all components and save those config params.

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Sep 3, 2016

Contributor

@brianteeman That's what we mean. Since the options aren't saved yet, there is no global value set for this parameter. Save them and it will show.

Contributor

Bakual commented Sep 3, 2016

@brianteeman That's what we mean. Since the options aren't saved yet, there is no global value set for this parameter. Save them and it will show.

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Sep 3, 2016

Contributor

So why are some set and others not - confused

On 3 September 2016 at 21:57, Thomas Hunziker notifications@github.com
wrote:

@brianteeman https://github.com/brianteeman That's what we mean. Since
the options aren't saved yet, there is no global value set for this
parameter. Save them and it will show.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11911 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8YQDGC_PW0sGD9l18eIXhvXxXgCVks5qmd8wgaJpZM4J0Xtq
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Contributor

brianteeman commented Sep 3, 2016

So why are some set and others not - confused

On 3 September 2016 at 21:57, Thomas Hunziker notifications@github.com
wrote:

@brianteeman https://github.com/brianteeman That's what we mean. Since
the options aren't saved yet, there is no global value set for this
parameter. Save them and it will show.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11911 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8YQDGC_PW0sGD9l18eIXhvXxXgCVks5qmd8wgaJpZM4J0Xtq
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Sep 3, 2016

Contributor

Because nobody cared to add them to the installation SQL file (https://github.com/joomla/joomla-cms/blob/staging/installation/sql/mysql/joomla.sql#L507). That's where we initially set some options for the core extensions.
Technically, it's not needed as long as the default values are correctly set in the XML and PHP code.

Contributor

Bakual commented Sep 3, 2016

Because nobody cared to add them to the installation SQL file (https://github.com/joomla/joomla-cms/blob/staging/installation/sql/mysql/joomla.sql#L507). That's where we initially set some options for the core extensions.
Technically, it's not needed as long as the default values are correctly set in the XML and PHP code.

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Sep 3, 2016

Contributor

So the problem then is that we can obviously add them for new installs but
can't for upgrades

On 3 Sep 2016 10:35 p.m., "Thomas Hunziker" notifications@github.com
wrote:

Because nobody cared to add them to the installation SQL file (
https://github.com/joomla/joomla-cms/blob/staging/
installation/sql/mysql/joomla.sql#L507). That's where we initially set
some options for the core extensions.
Technically, it's not needed as long as the default values are correctly
set in the XML and PHP code.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11911 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8e3gBoiy2R_IpxOXoiaIx7QGkSpvks5qmegWgaJpZM4J0Xtq
.

Contributor

brianteeman commented Sep 3, 2016

So the problem then is that we can obviously add them for new installs but
can't for upgrades

On 3 Sep 2016 10:35 p.m., "Thomas Hunziker" notifications@github.com
wrote:

Because nobody cared to add them to the installation SQL file (
https://github.com/joomla/joomla-cms/blob/staging/
installation/sql/mysql/joomla.sql#L507). That's where we initially set
some options for the core extensions.
Technically, it's not needed as long as the default values are correctly
set in the XML and PHP code.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11911 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8e3gBoiy2R_IpxOXoiaIx7QGkSpvks5qmegWgaJpZM4J0Xtq
.

@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Sep 4, 2016

Contributor

I have tested this item successfully on 9f49e39


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

Contributor

alikon commented Sep 4, 2016

I have tested this item successfully on 9f49e39


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

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Sep 4, 2016

Contributor

So the problem then is that we can obviously add them for new installs but
can't for upgrades

Indeed. However sometimes it's even forgotten for new installs. As far as I know it doesn't matter at all if the options are set or not, as long as the default values specified in the XMLs are the same as the one used in the PHP code. It only surfaced now with this PR.

Contributor

Bakual commented Sep 4, 2016

So the problem then is that we can obviously add them for new installs but
can't for upgrades

Indeed. However sometimes it's even forgotten for new installs. As far as I know it doesn't matter at all if the options are set or not, as long as the default values specified in the XMLs are the same as the one used in the PHP code. It only surfaced now with this PR.

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Sep 4, 2016

Contributor

The problem is that it makes accepting this PR problematic

On 4 September 2016 at 12:06, Thomas Hunziker notifications@github.com
wrote:

So the problem then is that we can obviously add them for new installs but
can't for upgrades

Indeed. However sometimes it's even forgotten for new installs. As far as
I know it doesn't matter at all if the options are set or not, as long as
the default values specified in the XMLs are the same as the one used in
the PHP code. It only surfaced now with this PR.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11911 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8UO_ZVGMKsH1BqSDMPI9rDKHUtzLks5qmqZFgaJpZM4J0Xtq
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Contributor

brianteeman commented Sep 4, 2016

The problem is that it makes accepting this PR problematic

On 4 September 2016 at 12:06, Thomas Hunziker notifications@github.com
wrote:

So the problem then is that we can obviously add them for new installs but
can't for upgrades

Indeed. However sometimes it's even forgotten for new installs. As far as
I know it doesn't matter at all if the options are set or not, as long as
the default values specified in the XMLs are the same as the one used in
the PHP code. It only surfaced now with this PR.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11911 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8UO_ZVGMKsH1BqSDMPI9rDKHUtzLks5qmqZFgaJpZM4J0Xtq
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Sep 4, 2016

Contributor

Yep, but unfortunately I don't see anything we can do here.
The default value for the formfield itself is usualy empty (means "Use Global"), thus we would have to parse the components config.xml to get the real default values. However that is just massively overkill and certainly shouldn't be done in the formfield class.

I thought about showing a notice which tells the user to save the options but we don't know if the param is just not saved yet or if it doesn't exist at all. So that's no option as well.

If someone has a good idea, I'm all ears 😄

I would also prefer to have the code in the JLayout instead of the formfield, but we're missing some data in there.

Contributor

Bakual commented Sep 4, 2016

Yep, but unfortunately I don't see anything we can do here.
The default value for the formfield itself is usualy empty (means "Use Global"), thus we would have to parse the components config.xml to get the real default values. However that is just massively overkill and certainly shouldn't be done in the formfield class.

I thought about showing a notice which tells the user to save the options but we don't know if the param is just not saved yet or if it doesn't exist at all. So that's no option as well.

If someone has a good idea, I'm all ears 😄

I would also prefer to have the code in the JLayout instead of the formfield, but we're missing some data in there.

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Sep 4, 2016

Contributor

hmm it looks like there is not anytime a translation form the internal integer to the actural value:
image

And this rendes not everytime:
image

The global value is:
image

Contributor

zero-24 commented Sep 4, 2016

hmm it looks like there is not anytime a translation form the internal integer to the actural value:
image

And this rendes not everytime:
image

The global value is:
image

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Sep 4, 2016

Contributor

The first one is because the component value is stored as int, while the option value is a string, and thus the strict comparision fails. Non strict fails as well because 0 is same as empty from "Use Global". So I will have to do some better checking there I guess.

Second one is because "Use Global" here references the Joomla Configuration, and not the component options. I'm not sure if we want to check against that one as well.

Contributor

Bakual commented Sep 4, 2016

The first one is because the component value is stored as int, while the option value is a string, and thus the strict comparision fails. Non strict fails as well because 0 is same as empty from "Use Global". So I will have to do some better checking there I guess.

Second one is because "Use Global" here references the Joomla Configuration, and not the component options. I'm not sure if we want to check against that one as well.

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Sep 4, 2016

Contributor

First one should be fixed now.

Contributor

Bakual commented Sep 4, 2016

First one should be fixed now.

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Sep 5, 2016

Contributor

I've moved the global value to the option itself like @andrepereiradasilva suggested above.
However to do that safely I had to add a new attribute useglobal which needs to be added to the field definition in the form xml. I did that for the article.xml where you see the changes needed.

The attribute itself already is used in other formfields, namely JFormFieldPlugintag and JFormFieldComponentlayout.

If this PR is accepted, it has to be done for the other extensions as well.

@zero-24 The robots field would now work as well as I have implemented a fallback lookup of the global config. However the value for the default option ("index, follow") is not the same in the global config and the article config, thus there is no matching. If you set the global value to something else, it will work.

Contributor

Bakual commented Sep 5, 2016

I've moved the global value to the option itself like @andrepereiradasilva suggested above.
However to do that safely I had to add a new attribute useglobal which needs to be added to the field definition in the form xml. I did that for the article.xml where you see the changes needed.

The attribute itself already is used in other formfields, namely JFormFieldPlugintag and JFormFieldComponentlayout.

If this PR is accepted, it has to be done for the other extensions as well.

@zero-24 The robots field would now work as well as I have implemented a fallback lookup of the global config. However the value for the default option ("index, follow") is not the same in the global config and the article config, thus there is no matching. If you set the global value to something else, it will work.

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Sep 5, 2016

Contributor

useglobal is more like showglobalvalue right?

i say this because if so, useglobal could be confused with adding the global option itself.

Contributor

andrepereiradasilva commented Sep 5, 2016

useglobal is more like showglobalvalue right?

i say this because if so, useglobal could be confused with adding the global option itself.

@sonalitailored

This comment has been minimized.

Show comment
Hide comment
@sonalitailored

sonalitailored Sep 5, 2016

I have tested this item successfully on 5d10a0f


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

I have tested this item successfully on 5d10a0f


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

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Sep 5, 2016

Contributor

i say this because if so, useglobal could be confused with adding the global option itself.

It does that as well 😄

Contributor

Bakual commented Sep 5, 2016

i say this because if so, useglobal could be confused with adding the global option itself.

It does that as well 😄

@hacki65

This comment has been minimized.

Show comment
Hide comment
@hacki65

hacki65 Sep 5, 2016

Contributor

It would be very nice if this request will be implemented. 👍

Contributor

hacki65 commented Sep 5, 2016

It would be very nice if this request will be implemented. 👍

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Sep 6, 2016

Contributor

It now shows a notice if there is no global value found for a field, letting the user know how he can solve the issue.
notice

@brianteeman Would that be helpful? Instead of showing one line for each field it could also just be a generic message. And of course feel free to suggest a better suited text 👍

Contributor

Bakual commented Sep 6, 2016

It now shows a notice if there is no global value found for a field, letting the user know how he can solve the issue.
notice

@brianteeman Would that be helpful? Instead of showing one line for each field it could also just be a generic message. And of course feel free to suggest a better suited text 👍

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Sep 6, 2016

Contributor

That is exactly what I was thinking of overnight - MUCH MUCH better

I'm excited about this feature

On 6 September 2016 at 07:50, Thomas Hunziker notifications@github.com
wrote:

It now shows a notice if there is no global value found for a field,
letting the user know how he can solve the issue.
[image: notice]
https://cloud.githubusercontent.com/assets/1018684/18264124/b8947334-740e-11e6-8e5c-7c64f61c5481.PNG

@brianteeman https://github.com/brianteeman Would that be helpful?
Instead of showing one line for each field it could also just be a generic
message. And of course feel free to suggest a better suited text 👍


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11911 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8Q_3VDVruMKAp7rImLO-IidPEqEDks5qnQ08gaJpZM4J0Xtq
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Contributor

brianteeman commented Sep 6, 2016

That is exactly what I was thinking of overnight - MUCH MUCH better

I'm excited about this feature

On 6 September 2016 at 07:50, Thomas Hunziker notifications@github.com
wrote:

It now shows a notice if there is no global value found for a field,
letting the user know how he can solve the issue.
[image: notice]
https://cloud.githubusercontent.com/assets/1018684/18264124/b8947334-740e-11e6-8e5c-7c64f61c5481.PNG

@brianteeman https://github.com/brianteeman Would that be helpful?
Instead of showing one line for each field it could also just be a generic
message. And of course feel free to suggest a better suited text 👍


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11911 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8Q_3VDVruMKAp7rImLO-IidPEqEDks5qnQ08gaJpZM4J0Xtq
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Sep 6, 2016

Contributor

@Bakual I think it needs to be a generic message - the user doesnt even need to know which option hasnt been set.

After saving the com_content options I still get

Unfortunately there was no global value found for the field 'Robots'. Saving the options may help to remedy this issue.

Contributor

brianteeman commented Sep 6, 2016

@Bakual I think it needs to be a generic message - the user doesnt even need to know which option hasnt been set.

After saving the com_content options I still get

Unfortunately there was no global value found for the field 'Robots'. Saving the options may help to remedy this issue.

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Sep 6, 2016

Contributor

Yeah, the robots field is strange. I tried to find a solution but the way we save and handle that parameter in particular is strange and inconsistent across all the code. That would need a bigger rewrite of how we handle that.
I think it is better to just remove the global value lookup for that field.

Made the notice now only appear once.
notice

Contributor

Bakual commented Sep 6, 2016

Yeah, the robots field is strange. I tried to find a solution but the way we save and handle that parameter in particular is strange and inconsistent across all the code. That would need a bigger rewrite of how we handle that.
I think it is better to just remove the global value lookup for that field.

Made the notice now only appear once.
notice

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Sep 6, 2016

Contributor

oh inconsistencies ... i have seen a few of those 😉

Contributor

andrepereiradasilva commented Sep 6, 2016

oh inconsistencies ... i have seen a few of those 😉

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Sep 6, 2016

Contributor

What do you think about adding some colour?

On 6 September 2016 at 11:56, andrepereiradasilva notifications@github.com
wrote:

oh inconsistencies ... i have seen a few of those 😉


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11911 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8dCwKsIqxCA73rVksQ9GYnfUc0fsks5qnUb7gaJpZM4J0Xtq
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Contributor

brianteeman commented Sep 6, 2016

What do you think about adding some colour?

On 6 September 2016 at 11:56, andrepereiradasilva notifications@github.com
wrote:

oh inconsistencies ... i have seen a few of those 😉


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11911 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8dCwKsIqxCA73rVksQ9GYnfUc0fsks5qnUb7gaJpZM4J0Xtq
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Sep 6, 2016

Contributor

first, i have to say that this PR is really really useful! really thanks @Bakual for making this.

Just some toughts about this:

  • i didn't checked, but, if i'm not wrong, this will make a joomla clean install get notices in almost all components (after you do the PRs for those). This could not be the best for newcomers. My opinion is that when we install something for the first time we don't expect to see that kind of notices, we expect a clean install. And even people that update will get those notices and can be confused. So, IMHO we should avoid using a message. Do you think there is a a way to detect this "problems" and save the component config default options in that case? This of course can be other PR.
  • Along with "Robots", the "Alternative Layout" field is still "Use Global". Never used that field, so i ask if this is the correct behaviour?
Contributor

andrepereiradasilva commented Sep 6, 2016

first, i have to say that this PR is really really useful! really thanks @Bakual for making this.

Just some toughts about this:

  • i didn't checked, but, if i'm not wrong, this will make a joomla clean install get notices in almost all components (after you do the PRs for those). This could not be the best for newcomers. My opinion is that when we install something for the first time we don't expect to see that kind of notices, we expect a clean install. And even people that update will get those notices and can be confused. So, IMHO we should avoid using a message. Do you think there is a a way to detect this "problems" and save the component config default options in that case? This of course can be other PR.
  • Along with "Robots", the "Alternative Layout" field is still "Use Global". Never used that field, so i ask if this is the correct behaviour?
@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Sep 6, 2016

Contributor

What do you think about adding some colour?

You mean like having the global value written in a different color?
I don't think we can apply styling to part of the select option. So it would be either the whole option (like we do for "Show" and "Hide") or none.
Also the tag doesn't allow any HTML tags within it, thus we can't add a or similar and style that.

Contributor

Bakual commented Sep 6, 2016

What do you think about adding some colour?

You mean like having the global value written in a different color?
I don't think we can apply styling to part of the select option. So it would be either the whole option (like we do for "Show" and "Hide") or none.
Also the tag doesn't allow any HTML tags within it, thus we can't add a or similar and style that.

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Sep 6, 2016

Contributor

To resolve the issue on clean installs the install.sql should be corrected
to include all the options (as it should always have been)

On 6 September 2016 at 12:49, andrepereiradasilva notifications@github.com
wrote:

first, i have to say that this PR is really really useful! really thanks
@Bakual https://github.com/Bakual for making this.

Just some toughts about this:

i didn't checked, but, if i'm not wrong, this will make a joomla clean
install get notices in almost all components (after you do the PRs for
those). This could not be the best for newcomers. My opinion is that when
we install something for the first time we don't expect to see that kind of
notices, we expect a clean install. And even people that update will get
those notices and can be confused. So, IMHO we should avoid using a
message. Do you think there is a a way to detect this "problems" and save
the component config default options in that case? This of course can be
other PR.

Along with "Robots", the "Alternative Layout" field is still "Use
Global". Never used that field, so i ask if this is the correct behaviour?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11911 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8VMuP9OuE59GCo1x8fw6tA46RN1-ks5qnVMygaJpZM4J0Xtq
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Contributor

brianteeman commented Sep 6, 2016

To resolve the issue on clean installs the install.sql should be corrected
to include all the options (as it should always have been)

On 6 September 2016 at 12:49, andrepereiradasilva notifications@github.com
wrote:

first, i have to say that this PR is really really useful! really thanks
@Bakual https://github.com/Bakual for making this.

Just some toughts about this:

i didn't checked, but, if i'm not wrong, this will make a joomla clean
install get notices in almost all components (after you do the PRs for
those). This could not be the best for newcomers. My opinion is that when
we install something for the first time we don't expect to see that kind of
notices, we expect a clean install. And even people that update will get
those notices and can be confused. So, IMHO we should avoid using a
message. Do you think there is a a way to detect this "problems" and save
the component config default options in that case? This of course can be
other PR.

Along with "Robots", the "Alternative Layout" field is still "Use
Global". Never used that field, so i ask if this is the correct behaviour?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11911 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8VMuP9OuE59GCo1x8fw6tA46RN1-ks5qnVMygaJpZM4J0Xtq
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Sep 6, 2016

Contributor

Along with "Robots", the "Alternative Layout" field is still "Use Global". Never used that field, so i ask if this is the correct behaviour?

The alternate layout field is not using JFormFieldList. It's using JFormFieldComponentlayout.
That's why it doesn't show the global value. It probably can be done there as well but likely needs a bit specialised code as it has to handle a grouped option list.
I would do that in another PR if we think it's needed.

Contributor

Bakual commented Sep 6, 2016

Along with "Robots", the "Alternative Layout" field is still "Use Global". Never used that field, so i ask if this is the correct behaviour?

The alternate layout field is not using JFormFieldList. It's using JFormFieldComponentlayout.
That's why it doesn't show the global value. It probably can be done there as well but likely needs a bit specialised code as it has to handle a grouped option list.
I would do that in another PR if we think it's needed.

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Sep 7, 2016

Contributor

Added the global values also to the menu items. com_content should be done everywhere now.

The single article menu item is a bit special as it uses radio buttons instead of the dropdown list for the options. The JFormFieldRadio would already support showing the global values because it extends JformFieldList. However it will look ugly because the buttons get different sizes depending on the global value and thus they wouldn't align anymore.
I didn't add the global values for now there until someone with design skills has an idea how that could be solved.

Contributor

Bakual commented Sep 7, 2016

Added the global values also to the menu items. com_content should be done everywhere now.

The single article menu item is a bit special as it uses radio buttons instead of the dropdown list for the options. The JFormFieldRadio would already support showing the global values because it extends JformFieldList. However it will look ugly because the buttons get different sizes depending on the global value and thus they wouldn't align anymore.
I didn't add the global values for now there until someone with design skills has an idea how that could be solved.

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Oct 31, 2016

Contributor

@brianteeman @andrepereiradasilva Conflicts are solved, code is moved to JFormAbstractlist. The code itself is the same, just in another class now (due to com_fields).
I also added the useglobal attribute to some newly introduced parameters. So it should work as expected again.

Contributor

Bakual commented Oct 31, 2016

@brianteeman @andrepereiradasilva Conflicts are solved, code is moved to JFormAbstractlist. The code itself is the same, just in another class now (due to com_fields).
I also added the useglobal attribute to some newly introduced parameters. So it should work as expected again.

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Oct 31, 2016

Contributor

Can zou take a look into the Travis problems?

Contributor

zero-24 commented Oct 31, 2016

Can zou take a look into the Travis problems?

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Oct 31, 2016

Contributor

Codestyle issues fixed.

Contributor

Bakual commented Oct 31, 2016

Codestyle issues fixed.

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Oct 31, 2016

Contributor

I have tested this item successfully on 9da466c

worked as before.


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

Contributor

andrepereiradasilva commented Oct 31, 2016

I have tested this item successfully on 9da466c

worked as before.


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

@conconnl

This comment has been minimized.

Show comment
Hide comment
@conconnl

conconnl Nov 4, 2016

I have tested this item successfully on 9da466c

Tested successfully at PBF NL.

Found a issue not related to this PR, but found because of this PR.
Show Page Heading on the Page Display tab states "Use Global" but we can't find any Global Setting for this option.


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

conconnl commented Nov 4, 2016

I have tested this item successfully on 9da466c

Tested successfully at PBF NL.

Found a issue not related to this PR, but found because of this PR.
Show Page Heading on the Page Display tab states "Use Global" but we can't find any Global Setting for this option.


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

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Nov 4, 2016

Contributor

@conconnl go to menu component options 😉

image

Contributor

andrepereiradasilva commented Nov 4, 2016

@conconnl go to menu component options 😉

image

@conconnl

This comment has been minimized.

Show comment
Hide comment
@conconnl

conconnl Nov 4, 2016

Thanks, we were searching with three people over here.
We didn't expect it to find it in Menu Global Options.
Tested it again, it's working perfectly

conconnl commented Nov 4, 2016

Thanks, we were searching with three people over here.
We didn't expect it to find it in Menu Global Options.
Tested it again, it's working perfectly

@roland-d

This comment has been minimized.

Show comment
Hide comment
@roland-d

roland-d Nov 4, 2016

Contributor

Setting to RTC


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

Contributor

roland-d commented Nov 4, 2016

Setting to RTC


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

@joomla-cms-bot joomla-cms-bot added RTC and removed Language Change labels Nov 4, 2016

@roland-d roland-d merged commit 4ebf7c0 into joomla:staging Nov 4, 2016

3 checks passed

JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/drone the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Bakual Bakual deleted the Bakual:ShowGlobalValues branch Nov 4, 2016

@zero-24 zero-24 removed the RTC label Nov 4, 2016

@cpfeifer cpfeifer referenced this pull request in joomla-projects/user-experience Nov 7, 2016

Closed

Review Global Setting usability improvements #5

nvyush added a commit to nvyush/joomla-cms that referenced this pull request Nov 9, 2016

JFormFieldList shows the global values if present. (#11911)
* JFormFieldList shows the global values if present.

* Moving string to en-GB.ini (where JGLOBAL_USE_GLOBAL lives)

* Rewrite to include the global value in the option itself.
Introduce "useglobal" attribute for listfield.

* Show a notice when global values aren't found

* Removed robots from being looked up.
Notice is now generic and will only show once.

* Adding global values to menu items

* Changed menuitems for all com_content views

* Single Article Menu Item done.

* Adding default value to some text fields

* global values for featured view

* Rebased and moved code to JFormAbstractlist to solve conflicts with com_fields PR

* Added useglobal to some newly introduced parameters.

* Codestyle
@hacki65

This comment has been minimized.

Show comment
Hide comment
@hacki65

hacki65 Dec 19, 2016

Contributor

I have installed the latest staging 3.7.0 and saw that not all global values are present in e.g. article -> options - only if i save the global settings once. Is there a plan for new installations to fix this? And what happens for updates? I think this issue must keep in mind to solve.

Contributor

hacki65 commented Dec 19, 2016

I have installed the latest staging 3.7.0 and saw that not all global values are present in e.g. article -> options - only if i save the global settings once. Is there a plan for new installations to fix this? And what happens for updates? I think this issue must keep in mind to solve.

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Dec 19, 2016

Contributor

Yes, there are some other PRs that deal with the installation default value. For updates there is nothing we can do. Just save the options and it works, otherwise it looks as before.

Contributor

Bakual commented Dec 19, 2016

Yes, there are some other PRs that deal with the installation default value. For updates there is nothing we can do. Just save the options and it works, otherwise it looks as before.

@degobbis

This comment has been minimized.

Show comment
Hide comment
@degobbis

degobbis Mar 18, 2017

Contributor

You should only see it if there are no saved options, and then you should save them anyway. So I think it's a helpful message.
But of course it would be easy to remove and could also be done in a later PR if more people think it is unneeded.

One possible idea might be to append the value (not set) instead of a general message. I think a message should follow an interaction and in this case is not really helpful at this point. The value (not set) in the list helps me to make a decision when choosing, precisely where it is needed. In addition, it would be consistent to always indicate whether a value is set, or which value is set.

Contributor

degobbis commented Mar 18, 2017

You should only see it if there are no saved options, and then you should save them anyway. So I think it's a helpful message.
But of course it would be easy to remove and could also be done in a later PR if more people think it is unneeded.

One possible idea might be to append the value (not set) instead of a general message. I think a message should follow an interaction and in this case is not really helpful at this point. The value (not set) in the list helps me to make a decision when choosing, precisely where it is needed. In addition, it would be consistent to always indicate whether a value is set, or which value is set.

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Mar 18, 2017

Contributor

@degobbis This PR is already merged, there is no point commenting further 😄

One possible idea might be to append the value (not set) instead of a general message

That would not be accurate as the default value is still set. It's hardcoded in the code then, just not saved in the database yet.

Contributor

Bakual commented Mar 18, 2017

@degobbis This PR is already merged, there is no point commenting further 😄

One possible idea might be to append the value (not set) instead of a general message

That would not be accurate as the default value is still set. It's hardcoded in the code then, just not saved in the database yet.

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Mar 18, 2017

Contributor

I have added the new attribute to the doku here: https://docs.joomla.org/List_form_field_type please correct any errors there. Thanks 👍

Contributor

zero-24 commented Mar 18, 2017

I have added the new attribute to the doku here: https://docs.joomla.org/List_form_field_type please correct any errors there. Thanks 👍

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Mar 18, 2017

Contributor

useglobal (optional) if set to true, it will show the value that is set in the global configuration if found in the database.

@zero-24 You may want to add that it automatically adds a the option "Use Global (value)" to the list. So that doesn't have to be added in the XML anymore. Not sure how to word it.

Contributor

Bakual commented Mar 18, 2017

useglobal (optional) if set to true, it will show the value that is set in the global configuration if found in the database.

@zero-24 You may want to add that it automatically adds a the option "Use Global (value)" to the list. So that doesn't have to be added in the XML anymore. Not sure how to word it.

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Mar 18, 2017

Contributor

@Bakual what about:

useglobal (optional) if set to true, it will show the value that is set in the global configuration if found in the database like this: "Use Global (value)".

maybe @brianteeman can help with the wording?

Contributor

zero-24 commented Mar 18, 2017

@Bakual what about:

useglobal (optional) if set to true, it will show the value that is set in the global configuration if found in the database like this: "Use Global (value)".

maybe @brianteeman can help with the wording?

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