User tests: Successful: 2 andrepereiradasilva, conconnl Unsuccessful: 0
Pull Request for Issue #11855.
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.
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).
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.
Status | New | ⇒ | Pending |
Category | ⇒ | Language & Strings Administration Libraries |
Labels |
Added:
?
?
|
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
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
@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.
Love the idea but it doesnt seem to read everything
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.
@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.
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/
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.
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
.
I have tested this item
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.
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 upgradesIndeed. 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/
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.
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.
First one should be fixed now.
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.
useglobal
is more like showglobalvalue
right?
i say this because if so, useglobal
could be confused with adding the global option itself.
I have tested this item
i say this because if so, useglobal could be confused with adding the global option itself.
It does that as well
It would be very nice if this request will be implemented.
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.
@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
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/
@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.
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.
oh inconsistencies ... i have seen a few of those
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/
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?
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.
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/
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.
Category | Language & Strings Administration Libraries | ⇒ | Administration Feature Request Language & Strings Libraries |
Labels |
Added:
?
|
Labels |
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.
Neither do I
I agree radios are nicer when having only a few options, but then consistency trumps it imho.
I think I will do a PR to correct that inconsistent view
Labels |
Added:
?
|
Done single article menu item as well (thanks @brianteeman
Also rebased the PR on the 3.7.x
branch.
Labels |
Removed:
?
|
This has NOT been merged yet - it still needs tests
That would be an idea. I can't do it this week as I'm in holidays but I can have a look next week
Milestone |
Added: |
Hi Guys, i'm really happy that this PR will come with 3.7.
But i will suggest a change for the view...
In my Feature Request the actual setting was at the right side of the dropdown.
And this is my favorite view. Why? Longer translation strings could snippet away if the value is in the dropdown itself. This happens e.g. for the robots values.
I think this should be in mind for the developing of this feature...
Having it on the right side wasn't looking nice in some cases (eg multiple columns of parameters). Also there is a chance of breaking layouts when the field is adding a span behind the actual dropdown. So that wasn't ideal. The current implementation works much better and is almost certainly not breaking anything.
The robots field doesn't work anyway due to the way the robots parameter works.
@andrepereiradasilva I have added the default values to the JFormFieldText as a placeholder (hint). I have used the same language string for now. Looks like this:
I think that is quite clear what the placeholder does and is consistent with the other places.
There is one question: If both a hint
and useglobal
is specified in the XML, what should this do? Currently useglobal
overwrites the hint. Imho it doesn't make sense to enbable both and thus we can leave it that way.
yeah, i think useglobal
should override the hint
if both exist.
please add them too to the Feature articles menu item layout
Done
I have tested this item
still think there should be no notice message (it's confusing).
But ... works as described. so tested with success.
still think there should be no notice message (it's confusing).
Why do you think it is confusing? Could it be better worded?
i prefer no message at all. but that's just me.
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.
Labels |
Added:
?
Removed: ? |
@Bakual can you fix conflicts.
@zero-24 @brianteeman IMO this is a very good usability feature for 3.7.0. Can you also help testing this?
I have tested it @andrepereiradasilva
@bakual can you fix the conflicts please
I fear due to the way the custom fields PR changed the formfields it is not a simple merge conflict. I have to move the whole code block to another class which means retesting of the whole thing.
It's not something I have time to do today but I can look at it next week.
@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.
Can zou take a look into the Travis problems?
Codestyle issues fixed.
I have tested this item
worked as before.
I have tested this item
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.
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
Status | Pending | ⇒ | Ready to Commit |
Labels |
Labels |
Setting to RTC
Category | Language & Strings Administration Libraries Feature Request | ⇒ | Administration Components Language & Strings Front End Libraries Feature Request |
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.
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.
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.
@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.
I have added the new attribute to the doku here: https://docs.joomla.org/List_form_field_type please correct any errors there. Thanks
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.
@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?
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.