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

Added stopPropagation to "copy text" buttons in backend #4019

Merged
merged 3 commits into from
May 29, 2024

Conversation

Caprico85
Copy link
Contributor

Description (*)

This PR adds event.stopPropagation() and event.preventDefault() to the new copyText() function.

The copy function is a very nice feature, which we already added to several other locations. Now our editors wanted to be able to copy values from backend grids, like this:

image

Adding the copy button can be done using a renderer. But the copy button is clicked, the event propagates up the DOM and triggers the row click, which navigates to the details page.

This is not what we wanted. I think, clicking the copy button should just do the copying and not do anything else. Therefore, calling event.stopPropagation() and event.preventDefault() to prevent all other actions should have no negative side effects.

Manual testing scenarios (*)

  1. A dirty way to add a copy button to a grid would be to edit Mage_Adminhtml_Block_Widget_Grid_Column_Renderer_Datetime::render() and add the line
    $data = '<span data-copy-text="' . $data . '">' . $data . '</span>';
    right before the return $data;, like this

    image
    This adds a copy button to the date column in the orders grid
    image

  2. Clicking the copy button copies the date. But it also navigates to the order details. With this PR applied, it only copies the date.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions github-actions bot added the JavaScript Relates to js/* label May 28, 2024
@fballiano
Copy link
Contributor

@hirale any thoughts on this?

@pauldpauld
Copy link

pauldpauld commented May 29, 2024

@Caprico85
Can I ask, did your "we already added to several other locations" include the order address block? I ask as I have been trying to to add those address fields with no success, a hint or more would be appreciated :-)

@Caprico85
Copy link
Contributor Author

Caprico85 commented May 29, 2024

@pauldpauld

Can I ask, did your "we already added to several other locations" include the order address block?

It does, but we use a modified template for that.

By default, the address is only shown with basic formatting:
image

We overrided the template app/design/adminhtml/default/default/template/sales/order/view/info.phtml and changed the address block to this

<table cellspacing="0" class="form-list">
	<tr>
		<td class="label"><label><?php echo $this->helper('enhancedsales')->__('Company') ?></label></td>
		<td class="value"><strong><?php echo $_order->getShippingAddress()->company ?></strong></td>
	</tr>
	<tr>
		<td class="label"><label><?php echo Mage::helper('sales')->__('Contact Person') ?></label></td>
		<td class="value"><strong><?php echo $this->escapeHtml($customerName); ?></strong></td>
	</tr>
	<tr>
		<td class="label"><label><?php echo Mage::helper('sales')->__('Street Address') ?></label></td>
		<td class="value"><strong><?php echo $_order->getShippingAddress()->street ?></strong></td>
	</tr>
	<tr>
		<td class="label"><label><?php echo Mage::helper('sales')->__('City') ?></label></td>
		<td class="value"><strong><?php echo $_order->getShippingAddress()->postcode ?> <?php echo $_order->getShippingAddress()->city ?></strong></td>
	</tr>
	<tr>
		<td class="label"><label><?php echo Mage::helper('sales')->__('Country') ?></label></td>
		<td class="value"><strong><?php  echo $_order->getShippingAddress()->getCountryModel()->getName() ?></strong></td>
	</tr>
	<?php if ($_order->getShippingAddress()->telephone): ?>
		<tr>
			<td class="label"><label><?php echo Mage::helper('sales')->__('Telephon') ?></label></td>
			<td class="value"><strong data-copy-text="<?php echo $_order->getShippingAddress()->telephone ?>"><?php echo $_order->getShippingAddress()->telephone ?> </strong></td>
		</tr>
	<?php endif; ?>
	<?php if ($_order->getShippingAddress()->fax): ?>
		<tr>
			<td class="label"><label><?php echo Mage::helper('sales')->__('Fax') ?></label></td>
			<td class="value"><strong><?php echo $_order->getShippingAddress()->fax ?></strong></td>
		</tr>
	<?php endif; ?>
</table>

(heavily modified Magento, so you may not be able to use the template as is)

Now our address looks like this:
image

You can see on the telephone line how we added the copy button.

@pauldpauld
Copy link

Thanks @Caprico85 That looks to be VERY helpful!

@hirale
Copy link
Contributor

hirale commented May 29, 2024

@hirale any thoughts on this?

It's a good idea.

@fballiano fballiano changed the title Prevent other actions from running when clicking the copy text button Added stopPropagation to "copy text" buttons in backend May 29, 2024
@fballiano fballiano merged commit 6cc06b3 into OpenMage:main May 29, 2024
14 checks passed
@Caprico85 Caprico85 deleted the copytext-prevent branch May 29, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JavaScript Relates to js/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants