OpenMage / magento-lts

Official OpenMage LTS codebase | Migrate easily from Magento Community Edition in minutes! Download the source code for free or contribute to OpenMage LTS | Security vulnerability patches, bug fixes, performance improvements and more.
https://www.openmage.org
Open Software License 3.0
863 stars 438 forks source link

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

Closed Caprico85 closed 1 month ago

Caprico85 commented 1 month ago

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 (*)

fballiano commented 1 month ago

@hirale any thoughts on this?

pauldpauld commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

Thanks @Caprico85 That looks to be VERY helpful!

hirale commented 1 month ago

@hirale any thoughts on this?

It's a good idea.