Skip to content
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

[com_fields] Fix text-value being reverse for fields: list, radio, checkbox. Fix fields: radio, checkboxes not showing #12739

Merged
merged 8 commits into from
Nov 7, 2016

Conversation

ggppdk
Copy link
Contributor

@ggppdk ggppdk commented Nov 3, 2016

Pull Request for Issue #12725

Summary of Changes

  • Fix text-value pair being reversed for fields: list, radio, checkbox, (both in form and when viewing)
  • Fix fields: radio, checkboxes not showing (...)
  • Fix label of checkboxes (field type selector) showing as 'Checkbox'
  • Change Label 'Name' to 'Text' for List Field configuration for "Add values" popup
  • Change Label 'Key' to 'Text' for Radio / Checkboxes Fields configuration for "Add values" popup
  • Added missing display layout file for radio field

NOTE: zero (single character '0') is not yet usable as field value, do not use it yet (there will be another)

(...) they used name="key" inside their XML file, instead of name="name" that php code expects in:
libraries/joomla/form/abstractlist.php

Testing Instructions

  1. Create 3 fields: radio, checkboxes, list and add values to them (important if you have created them already then redit the values and save)
  2. Add values to the 3 fields in article form and save
  3. View the article in frontend

Documentation Changes Required

None

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-staging and removed Language Change This is for Translators labels Nov 3, 2016
@Bakual
Copy link
Contributor

Bakual commented Nov 6, 2016

Homestly I'm lost by now. The option text is already translated fine when this PR is applied (without it's messed up).
See my options
list-options

Then how it appears in the article edit form
test-list

And finally in frontend in the article view
frontend

@Bakual
Copy link
Contributor

Bakual commented Nov 6, 2016

So the code is there to show the proper text based on value and even translate it. Nothing more is needed.

Using such custom fields NEED specific layouts in php where the new parameters are used to do something or another.

Keep in mind custom fields areen't meant to change behavior. Like change how the layout appears or what it is supposed to do. The fields are meant to add custom data. Like if you have a group of articles about historic events, you could add a date of the event using a custom calendar field. That date is just going to be shown in frontend.

@brianteeman
Copy link
Contributor

Honestly I'm lost by now
Me too

@ggppdk
Copy link
Contributor Author

ggppdk commented Nov 6, 2016

Homestly I'm lost by now. The option text is already translated fine when this PR is applied (without it's messed up).

Because i added it in a today's commit, see the commit here: 141309f

I added it, after last comments from @infograf768, and after my comment, that the FORM HTML of the fields: radio / checkboxes / list, already assume that "Option Text" need to be JTexted and do it already, so JText for "Option Texts" should be in the view layout too, without a parameter


This PR makes multiple but small changes, for the JFormAbstractlist fields: radio, checkboxes, list

  • corrected a few language strings, so that the 3 fields are consistent
  • changed (wrong) parameter name "key" to "name" in XML files, that caused fields: radio / checkboxes not to show in the form
  • reversed the "name"(=Option text), "value"(Option value) PAIR in JFormAbstractlist ::getOptionsFromField(), so that "Option Text" is displayed instead of "Option Value"
  • added a layout for radio field (same as fields: checkboxes, list)
  • added JText to the Options-text in the field's viewing layouts

@Bakual
Copy link
Contributor

Bakual commented Nov 6, 2016

Because i added it in a today's commit

Oh, I thought that one worked before as well, looks I was wrong.

@infograf768
Copy link
Member

we still need to Jtext the label

@ggppdk
Copy link
Contributor Author

ggppdk commented Nov 6, 2016

we still need to Jtext the label

right, yes,

But JText the label must be a different PR right ? because it is something that effects all fields

Like the "0" character as value, is not showing (single value selected), should be a different PR, because it effects all fields too, (i had fix here but it was undone)

Otherwise this PR will keep needing retesting, lets keep this PR to effect JFormAbstractlist fields: radio, checkboxes, list

@Bakual
Copy link
Contributor

Bakual commented Nov 6, 2016

This PR does already more than what it was supposed to do. Please don't add the label and the 0 issue here or we can never merge it.

@Bakual
Copy link
Contributor

Bakual commented Nov 6, 2016

I've tested this with all three types now. They all share an issue in frontend if a 0 value is selected:

  • If no default value is set, they disappear completely
  • If a default value is set, that default value is used instead of the 0 value.

I think that was the bug you solved and reverted due to miscommunication between the two of us?
This issue is specific to com_fields and could be solved with this PR or another one. I don't mind much.

The one I referred earlier was when 0 was used as the text not the value. Then the text is ignored and the value is used as text. Like when no text is given. But that is an issue unrelated to com_fields.

@Bakual
Copy link
Contributor

Bakual commented Nov 6, 2016

Checkboxes have an additional issue:
If the 0 value is the only one selected, then it will be unselected after saving. If you select the 0 value together with another value, then it gets saved properly.
This is an issue with com_fields only but probably also unrelated to this PR. So could be done in another PR as well.

@ggppdk
Copy link
Contributor Author

ggppdk commented Nov 6, 2016

  • If no default value is set, they disappear completely
  • If a default value is set, that default value is used instead of the 0 value.
    I think that was the bug you solved and reverted due to miscommunication between the two of us?

yes exactly

This issue is specific to com_fields

yes

Checkboxes have an additional issue:
If the 0 value is the only one selected, then it will be unselected after saving. If you select the 0 value together with another value, then it gets saved properly.

Yes,
!$this->value returns false if it is single value
!$this->value returns true if it is an array of values that one of them is "0'

Because the layout of all files and the helper class need to be to patch: see comment above: #12739 (comment)

Let's put it in a different PR together with the label you can do or i will do tomorrow

@ggppdk ggppdk force-pushed the fix_text_value_fields branch from 141309f to b121e63 Compare November 6, 2016 20:51
@joomla-cms-bot joomla-cms-bot removed the Language Change This is for Translators label Nov 6, 2016
@infograf768
Copy link
Member

label PR is here: #12656

@Bakual
Copy link
Contributor

Bakual commented Nov 7, 2016

We need to merge this PR first to solve the other issues properly. From my PoV this is RTC now, but since I'm tester someone else should merge it :)

@Bakual Bakual added this to the Joomla 3.7.0 milestone Nov 7, 2016
@rdeutz rdeutz merged commit fc0fd25 into joomla:staging Nov 7, 2016
@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Nov 7, 2016
@ggppdk ggppdk deleted the fix_text_value_fields branch November 7, 2016 10:51
nvyush pushed a commit to nvyush/joomla-cms that referenced this pull request Nov 9, 2016
…eckbox. Fix fields: radio, checkboxes not showing (joomla#12739)

* Fix value-text being reverse for fields list, radio, checkbox, change 'name','key' label to be 'text' for these fields

* Fixed calling method from parent class

* Added missing layout file for radio field

* Fixed character '0' unusuable as field value

* Revert fix for zero character as value

* Proper variable names for view layouts of options-using fields

* They are options not just values

* JText the Text of options in the view layouts, since respective form fields do it already
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants