-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Fixes the active_tab param not applying on admin sections that use th… #35779
base: 2.4-develop
Are you sure you want to change the base?
Conversation
Hi @jajajaime. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review. For more details, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket. ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
@@ -65,7 +65,7 @@ define([ | |||
this.anchors : | |||
this._getList().find('> li > a[href]'); | |||
|
|||
return anchors.index($('#' + id)); | |||
return anchors.index($('a#' + id)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Identifiers must be unique on each page. Adding a qualifier to something that's already unique doesn't seem to make sense to me. Perhaps the fix for the problem that you're experiencing is to remove elements or change identifiers to resolve the duplicate problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I'm following when you say "duplicate". I am just trying to grab the element with the correct identifier that the find returns.
For example above on a product attribute edit page, looking at the anchors that get found, they're all in the form of a#product_attribute_tabs_X
Looking at the id
variable, the index
method doesn't find anything and always returns a -1
, because it can't find #product_attribute_tabs_X
, as the a
is in front of it.
This makes all edit pages that use the tabs.js
script to never be able to link directly to a tab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$
here is jQuery. Calling jQuery()
as a function like this takes a CSS selector as its first argument. The string that's being generated / used here is in the form #identifier
, which is an ID selector as described here: https://developer.mozilla.org/en-US/docs/Web/CSS/ID_selectors
See here for information about the id
attribute. https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id (This page is one of many that states that IDs must be unique within a document / page.)
It sounds like this bug requires further investigation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you're saying. The li
and child a
elements have the same identifier, making the index not find it.
That identifier defined in the backend extension in
https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/Backend/view/adminhtml/templates/widget/tabs.phtml#L34
and line 37 for the a
.
So in reality, probably the actual solution would be to give the a
tag a different identifier, and look for that new identifier when trying to find the index in the tabs.js
widget.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that sounds like the right thing to do. Or change (remove?) the id
attribute for the list item (<li>
) and leave the anchor (<a>
) with its current ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tabs fail to load if the id is different or missing on the <li>
element, it looks like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that sounds like the right thing to do. Or change (remove?) the
id
attribute for the list item (<li>
) and leave the anchor (<a>
) with its current ID.
Magento/Backend/view/adminhtml/templates/widget/tabs.phtml#L34
class="admin__page-nav-item no-display"
id="<?= $block->escapeHtmlAttr($block->getTabId($_tab)) ?>"
=>
class="admin__page-nav-item no-display"
id="<?= $block->escapeHtmlAttr($block->getTabId($_tab)) ?>_nav_item"
$('li.admin__page-nav-item#{$block->escapeJs($block->getTabId($_tab))}')
=>
$('li.admin__page-nav-item#{$block->escapeJs($block->getTabId($_tab))}_nav_item')
Any update on this PR? |
…e tabs.js script
Description (*)
On Admin sections like Sales Order Edit, Product Attribute Edit, Admin User Edit. among others, that use the
tabs.js
mage script, there exists supporting code to allow for anactive_tab
to be passed in the URL as a param, and that tab will get selected and load first on the page. This is all working up until the JS has to find the selector and its index on the page, where it is using the wrong selector, so it always gets an index of-1
.Manual testing scenarios (*)
/active_tab/order_shipments
and go to that pageShipments
tab active/active_tab/order_invoices
. This should select theInvoices
tab on loadContribution checklist (*)