Skip to content

Conversation

@infograf768
Copy link
Member

@infograf768 infograf768 commented Aug 9, 2016

We do need the id of the menu item in the $this->fields = array( in a menu item edit.php) for the GSOC multilingual project.
It was missing anyway to display the id in the page.

After patch, the readonly field will display:
screen shot 2016-08-09 at 07 59 50

Can be merged on review.
@andrepereiradasilva @alikon @jreys
@wilsonge

@alikon
Copy link
Contributor

alikon commented Aug 9, 2016

tested ok
but unable to mark successfull test on issues.joomla.org

@infograf768
Copy link
Member Author

I marked it for you

@jreys
Copy link
Contributor

jreys commented Aug 9, 2016

I have tested this item ✅ successfully on ab4c091


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

@infograf768
Copy link
Member Author

@wilsonge

Thank you for merging


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 9, 2016
@jeckodevelopment
Copy link
Member

I have tested this item ✅ successfully on ab4c091


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

@wilsonge
Copy link
Contributor

wilsonge commented Aug 9, 2016

What is this needed for? Seems kind of counterintuitive to the UI?

@jreys
Copy link
Contributor

jreys commented Aug 9, 2016

@wilsonge , this is needed in order to save associations on the side-by-side, without jform_id, I can't save new associations since I don't know which ID was generated

@alikon
Copy link
Contributor

alikon commented Aug 9, 2016

I have tested this item ✅ successfully on ab4c091

test to test joomla/jissues#860


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

'language',
'note'

'note',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just by code review (i.e. not tested...): are you sure that comma after the last array element is correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I found this: http://stackoverflow.com/questions/2829581/why-do-php-array-examples-leave-a-trailing-comma

In essence, valid syntax, although unusual, and good for convenience in case you wish to add one more element in the code at a later time.

@brianteeman
Copy link
Contributor

Does this field need to be displayed in the UI. Doesnt seem any need for it to be displayed to me and just adds meaningless clutter


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

@infograf768
Copy link
Member Author

It does NOT need to be displayed but it is useful information that can be copied and does not clutter as it is at bottom of all fields.

@wilsonge
Copy link
Contributor

OK Can we make this a hidden field on the page - with an ID on the hidden field available for JS access

@infograf768
Copy link
Member Author

@wilsonge
Done. imho does not need to be tested again. Can be merged on review.

@brianteeman
Copy link
Contributor

Thank you for making that update

@wilsonge wilsonge merged commit 2a0fb86 into joomla:staging Aug 13, 2016
@wilsonge wilsonge added this to the Joomla 3.6.3 milestone Aug 13, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 13, 2016
ggppdk pushed a commit to ggppdk/joomla-cms that referenced this pull request Aug 19, 2016
* Needed for GSOC: Adding id in menu item edit array

* hiding id field
roland-d pushed a commit to roland-d/joomla-cms that referenced this pull request Sep 11, 2016
* Needed for GSOC: Adding id in menu item edit array

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants