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

modal_menu: Displaying the correct placeholder from the option #16459

Merged
merged 1 commit into from
Jun 6, 2017

Conversation

infograf768
Copy link
Member

@infograf768 infograf768 commented Jun 3, 2017

Pull Request for Issue #16421

Summary of Changes

Adding conditionals to display the default placeholder with null value defined in the field Option

Testing Instructions

Edit an item which contains a choice for a menu item field using the modal_menu type.

  1. Creating a Menu Item Alias. Choosing the menu item to alias to.
  2. Creating a Create Article menu item. Choosing the menu item Submission Redirect in Options.
  3. Creating a Login Form menu item. Choosing the Menu Item Login Redirect and Menu Item Logout Redirect in the Options.
  4. Creating a Logout menu item. Choosing the Logout Redirection Page in the Options.
  5. Editing the Site Login Form module. Choosing the Login Redirection Page and the Logout Redirection Page.
  6. Editing the Site Menu module. Choosing the Base Item.

The xml code for the Menu module is for example

				<field
					name="base"
					type="modal_menu"
					label="MOD_MENU_FIELD_ACTIVE_LABEL"
					description="MOD_MENU_FIELD_ACTIVE_DESC"
					select="true"
					new="true"
					edit="true"
					clear="true"
					>
					<option value="">JCURRENT</option>
				</field>

In each case, select a menu item in the modal. Then Clear that menu item.

Expected result for the placeholder value

  1. Select a Menu Item
  2. Default
  3. Default for both
  4. Default
  5. Default for both
  6. Current

Actual result

All have Select a Menu Item

Patch and test again. Do not forget to choose a menu item and Clear to confirm that the correct value is displayed.

Here is an example —after patch— concerning the original issue with Menu module.

menumodalfield

@epidote

@AlexRed
Copy link
Contributor

AlexRed commented Jun 3, 2017

I have tested this item ✅ successfully on 113c374

Patch ok for me


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

@Bakual
Copy link
Contributor

Bakual commented Jun 3, 2017

Looks a bit wrong to me to use the <option> tag for this. The tag could be present more than once but your code only supports one. So imho it would make more sense to use an attribute (eg placeholder or whatever you like) of the field instead a child element.

@infograf768
Copy link
Member Author

infograf768 commented Jun 3, 2017

this pr deals with existing xmls. its purpose was not to refactor them.
if you think we should, then let's do this, but it means modifying each of them.

btw, the option is NOT supposed to be used more than once per field.

@Bakual
Copy link
Contributor

Bakual commented Jun 3, 2017

Ah sorry, yes then of course we need to take what is there already.

@carcam
Copy link

carcam commented Jun 3, 2017

I have tested this item ✅ successfully on 113c374


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

@ghost
Copy link

ghost commented Jun 3, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 3, 2017
@infograf768 infograf768 added this to the Joomla 3.7.3 milestone Jun 4, 2017
@rdeutz rdeutz merged commit c3f9ac6 into joomla:staging Jun 6, 2017
@joomla-cms-bot joomla-cms-bot added PR-staging and removed RTC This Pull Request is Ready To Commit labels Jun 6, 2017
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.

6 participants