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

Backend: Improved "copy button" style in legacy admin theme #4072

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

ma4nn
Copy link
Contributor

@ma4nn ma4nn commented Jul 3, 2024

Description (*)

With the latest addition of the copy button feature, the order header in the legacy admin theme looks a bit broken:
Bildschirmfoto 2024-07-03 um 08 59 10

This PR improves the styling of the button in that section:
Bildschirmfoto 2024-07-03 um 08 58 31

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

  1. Copy button looks nice in legacy and openmage admin theme
  2. Copy button functionality is still present

Questions or comments

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 Template : admin Relates to admin template Component: Adminhtml Relates to Mage_Adminhtml labels Jul 3, 2024
@ADDISON74
Copy link
Contributor

ADDISON74 commented Jul 3, 2024

I just wanted to make an observation on this topic because I saw the change with the new v20 version.

In CSS you should your position: absolute then you can arrange the icon into the right position. This is a test for the email icon

.icon-copy {
    display: inline-block;
    background-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 111.07 122.88"><path d="M97.67 20.81h0l.01.02a13.38 13.38 0 0 1 9.46 3.93 13.32 13.32 0 0 1 3.9 9.42h.02v.02 75.28.01h-.02c-.01 3.68-1.51 7.03-3.93 9.46a13.32 13.32 0 0 1-9.42 3.9v.02h-.02-59.19-.01v-.02c-3.69-.01-7.04-1.5-9.46-3.93a13.37 13.37 0 0 1-3.91-9.42h0V34.2v-.01h.02c.01-3.69 1.52-7.04 3.94-9.46 2.41-2.4 5.73-3.9 9.42-3.91v-.02h.02l59.17.01h0zM.02 75.38L0 13.39v-.01h.02a13.44 13.44 0 0 1 3.93-9.46A13.37 13.37 0 0 1 13.37.01V0h.02 59.19c7.69 0 8.9 9.96.01 10.16H13.4h-.02v-.02c-.88 0-1.68.37-2.27.97-.59.58-.96 1.4-.96 2.27h.02v.01 3.17 58.81c0 8.26-10.15 8.72-10.15.01h0zm100.89 34.11V34.2v-.02h.02c0-.87-.37-1.68-.97-2.27-.59-.58-1.4-.96-2.28-.96v.02h-.01-59.19-.02v-.02c-.88 0-1.68.38-2.27.97-.59.58-.96 1.4-.96 2.27h.02v.01 75.28.02h-.02c0 .88.38 1.68.97 2.27s1.4.96 2.27.96v-.02h.01 59.19.02v.02c.87 0 1.68-.38 2.27-.97.59-.58.96-1.4.96-2.27h-.01 0 0 0z" fill-rule="evenodd"/></svg>');
    background-repeat: no-repeat;
    background-size: contain;
    cursor: pointer;
    width: 12px;
    height: 12px;
    margin: 3px 0 0 6px;
    position: absolute;

and the same width, height and margin for the order icon. In this way both icons will look good. The color for the order must be the same with text. For the email it should be evaluate orange from the email or black from the text.

@ma4nn
Copy link
Contributor Author

ma4nn commented Jul 3, 2024

@ADDISON74 I would not recommend using position: absolute, especially without any relative container outside.
Your proposed color changes are already part of this PR 🙂

But having a closer look at the copy code button implementation, I would improve the following:

  • span should be a button
  • the data attribute should not be the only selector but rather a css class in addition
  • onclick should be an event listener
  • code is not admin related, so it should be in a more general JS file in js/mage

@fballiano fballiano changed the title Improve copy button style in legacy admin theme Backend: Improved "copy button" style in legacy admin theme Jul 15, 2024
@fballiano
Copy link
Contributor

I've tested your PR and it's needed so I'll merge it right away.

Personally I'd still keep the code in a "admin" related file but further improvements can be done for sure in a separate PR.

@fballiano fballiano merged commit fae22f5 into OpenMage:main Jul 15, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml Template : admin Relates to admin template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants