JFormOption #5147
JFormOption #5147
Conversation
|
Can you fix the codestyle errors? |
okonomiyaki3000
force-pushed
the
stg-JFormOption
branch
from
6c08f8a
to
dd092dc
Compare
|
Fix some merge conflicts in this old thing. This is still really cool, btw. |
|
Hmm looks like a unit test fails here? Can you have a look? https://travis-ci.org/joomla/joomla-cms/builds/82667974 |
…dList so that there is only one implementation of getOptions(). New class JFormOption and assorted option-type classes to be used to generate lists of options based on the <option/> xml element. Other related changes to JForm and JFormHelper.
okonomiyaki3000
force-pushed
the
stg-JFormOption
branch
from
dd092dc
to
bc0120d
Compare
|
There was a bug in my code but I also found that the unit test was laughable. So it's a bit better now. However, it's using the |
|
It's removed from PHPUnit 5. So either we need to eventually write something similar or pull in some third party library that has the ability to parse that stuff in a unit test run. |
|
I'd rather just see all html removed from server-side code but that's kind of a pipe dream. |
|
@okonomiyaki3000 can you delete this file: libraries/joomla/form/options/index.html? Joomla dropped all the blank .html files long time ago |
|
Yeah, I seem to have some vague recollection of that. |
|
@okonomiyaki3000 I know this is another old PR but I agree that this is still cool to implement. If you could update the PR to resolve the merge conflicts I will get some people to test this and we can implement it for 3.6. Thanks. |
|
@roland-d Thanks, I was thinking of getting back to this one soon anyway. I'm glad to know there's interest in it. The usefulness of it might not be immediately obvious to everyone but I happen to think it's pretty great. |
|
@okonomiyaki3000 I have set the milestone for now to 3.7.0 as we are too late for 3.6.0. |
|
I have tested this item 🔴 unsuccessfully on 742fa5e Tested @icampus This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5147. |
|
I have tested this item 🔴 unsuccessfully on 742fa5e This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5147. |
|
Make sure you've actually got that file in your local install. Should be present as of 3.5.0. |
|
Oh, nevermind, I see the issue now. This does need to be sync'd with staging it looks like; the file it's groaning about appears to have been renamed. |
|
Sorry guys, this is really old and needs a lot of work. I'm glad it's getting some attention again because I still think it's a great idea but I need to find some time to to fix it up. Also, I never took into account grouped lists. Really I don't know why grouped lists need to be any different from ungrouped lists. I'll need to have that in mind when updating this I guess. |
|
@okonomiyaki3000 Yeah I know this is old but you know by now that I tend to drag these things out into the light ;) Shall we close this issue for now and can be re-opened or a new one can be made when you have time for it? This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5147. |
|
New PR: #12288 |
|
@roland-d I finally got around to doing a little work on it. How do you think it's best to proceed? From here or with a new pr? Just so you know, I don't think it's quite ready to merge yet but anyway I'm ready to continue discussing and working on it. |
|
I have opend a new PR maybe you can fork that branch and do your own PR? |
|
OK, that sounds good. has some work been done on it then? I guess I'd better go see. |
|
https://github.com/zero-24/joomla-cms/tree/5147 I have just fixed the conflicts |
|
@zero-24 It looks like that branch needs to be rebased with staging. It might end up being a big hassle. I have a branch that's already been rebased, maybe it's best to make a new PR? or has some work been done on your branch already? |
|
No there is no additional work done. |
This is the same as #1192 but for staging instead of master.