-
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
[Framework] New Link is not correctly shown as Current if contains default parts #19134
[Framework] New Link is not correctly shown as Current if contains default parts #19134
Conversation
Hi @eduard13. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
@@ -81,7 +85,7 @@ private function getMca() | |||
*/ | |||
public function isCurrent() | |||
{ | |||
return $this->getCurrent() || $this->getUrl($this->getPath()) == $this->getUrl($this->getMca()); |
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.
This change might be harmful since we stop using getMca()
method at all. That means that we can have the current links functionality broken in different places. I would recommend adding an additional check here (by trimming /index/index
or so) instead of removing invoking of getMca()
method
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.
@rogyar thank you for suggestion, currently the logic of the getMca
method was improved, by removing the _defaultPath
logic (which was wrong in my opinion). Also there are covered all the cases when a default url part is used.
So having a new link test/index
, the following possible cases will mark this link as Current:
test
test/index
test/index/index
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.
Have you tested passing GET parameters via URL as well?
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, with all these cases - the link keeps its current state.
…for default parts
@rogyar is there anything else that I can do here? |
…/magento2 into 2.3-develop-19099-issue
Hi @eduard13. Thank you for your contribution. |
…f contains default parts #19134
Description (*)
This PR fixes the logic of
isCurrent
method by comparing the current requested url with the link path. This way we'll always know if the link is current, even it uses someindex
default parts.Fixed Issues (if relevant)
Manual testing scenarios (*)
Contribution checklist (*)