LPS-127831 Search Iterator display style icon should only use active … #821
Conversation
…class when highlighted instead of active table-active
|
CI is automatically triggering the following test suites:
|
✔️ ci:test:sf - 1 out of 1 jobs passed in 1 day 16 hours 53 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-127831 1 Successful Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#3205 |
✔️ ci:test:stable - 9 out of 9 jobs passed❌ ci:test:relevant - 20 out of 23 jobs passed in 6 hours 40 minutesClick here for more details.Base Branch:Branch Name: master Upstream Comparison:Branch GIT ID: d610360269471ce22dea81957a59d19a64492406 ci:test:stable - 9 out of 9 jobs PASSED9 Successful Jobs:
ci:test:relevant - 20 out of 23 jobs PASSED3 Failed Jobs:20 Successful Jobs:
For more details click here.Failures unique to this pull:
Failures in common with acceptance upstream results at d610360:
|
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#5469 |
Haven't looked at the code yet (about to do that), but going to run the tests in the meantime... |
|
ci:test:relevant |
|
ci:test:bundle |
| if (Lang.isString(rowClassNameActive)) { | ||
| var active = rowClassNameActive.split(','); | ||
| var rowSelector = [ | ||
| 'li[data-selectable="true"]', | ||
| 'tr[data-selectable="true"]', | ||
| ]; | ||
|
|
||
| return active[ | ||
| rowSelector.indexOf(this.get(STR_ROW_SELECTOR)) | ||
| ]; | ||
| } |
There was a problem hiding this comment.
I must confess that I don't actually understand how this works. Let me try to put it into words and you can confirm for me whether I've understood it:
-
Previously, the value here was
active table-active; now it isactive,table-active. -
There is no reference to
rowClassNameActiveoutside of this file, so just based on what I see in here, the value is going to get passed to intoaddClassortoggleClass. I can't find the docs or the implementation for YUI/AUIaddClass, but at least this line suggests to me that it expects a space-separated list of class names; I am not sure what it will do when given a comma-separated list... -
I don't know AUI/YUI, so I don't know if this
setteris magically called somehow; I can't see any setter calls anywhere (and like I said, can't see any references outside of this file). My guess here is that your intent is that yoursplit()call turns the invalid comma-containing "name" into a single name, but I have no idea when the setter is called. -
There's obviously a relationship between the classes in
rowSelectorbelow and the ones on line 81 above, but it seems subtle and fragile, and the whole thing seems roughly equivalent to this:return 'active,table-active'.split(',')[ [ 'li[data-selectable="true"]', 'tr[data-selectable="true"]', ].indexOf('li[data-selectable="true"],tr[data-selectable="true"]'), ];
I don't really get it, as that
indexOf()looks like it is always going to return-1, so the access by array-index of-1into the array will always returnundefined. Maybe somebody who knows AUI a bit better can review this? cc @jonmak08
There was a problem hiding this comment.
I must confess that I don't actually understand how this works.
You and me both. This is always magic to me.
I don't know if this setter is magically called somehow;
I think setter gets called via aui-component.js and widget-base.js (YUI), maybe these two files will clear up when it gets called? 🤷
There was a problem hiding this comment.
Well, I don't think I can forward this without understanding how it works, especially if you also say that you don't understand it. 😬 Can you think of anybody who would be better qualified to review it?
There was a problem hiding this comment.
To be honest, I don't know how or why would this work either 😂 @pat270, how did you come up with this script then? Trial and error?
Our main logic is in this same file some lines below:
var row = event.currentTarget.ancestor(
instance.get(STR_ROW_SELECTOR)
);
[...]
row.toggleClass(instance.get(STR_ROW_CLASS_NAME_ACTIVE));Where row is the li for cards and descriptive visualizations and the tr for tables. So...
Card View image card doesn't show active border because it is a special case where we can't style the card with the input:checked ~ .card selector or with the active class on the li element.
I'm not sure I understand what's the technical limitation here... 🤔
There was a problem hiding this comment.
So... shot in the dark here:
Is the goal of this PR to simply have the active class in the li and not both active and table-active?
(This doesn't fix the issue, which still needs the new Clay release merged, I think?)
There was a problem hiding this comment.
There are two issues:
-
https://issues.liferay.com/browse/LPS-127450 (Styles for active row is lost). Bootstrap 4 changed the
activeclass on table rows totable-active. The compat layer removal exposes this issue on search container table view. -
https://issues.liferay.com/browse/LPS-127831 (Extra styles are added to selected card). The fix from LPS-127450 caused this. The light blue border/background around the card (caused by
table-active) is what needs to be fixed.
The Clay pr will add the blue border around image card when the card is checked.
liferay-continuous-integration
added
ci:test:bundle - success
and removed
ci:test:bundle - pending
labels
✔️ ci:test:bundle - 1 out of 1 jobs passed in 41 minutesClick here for more details.Base Branch:Branch Name: master ci:test:bundle - 1 out of 1 jobs PASSEDFor more details click here.Test bundle downloads:
|
✔️ ci:test:stable - 9 out of 9 jobs passed✔️ ci:test:relevant - 22 out of 23 jobs passed in 1 hour 27 minutesClick here for more details.Base Branch:Branch Name: master Upstream Comparison:Branch GIT ID: aa80a9ec2a3bc90349b41faec6f2fa93e99fc0c4 ci:test:stable - 9 out of 9 jobs PASSED9 Successful Jobs:
ci:test:relevant - 21 out of 23 jobs PASSED2 Failed Jobs:21 Successful Jobs:
For more details click here.This pull contains no unique failures.Failures in common with acceptance upstream results at aa80a9e:
|
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#4302 |
|
Closing in favour of #837. @pat270, couldn't push to your fork. Could you please check point 9 of the Frontend Infrastructure Team code review process to expedite this in the future?
/cc @wincent, might wanna hide Bruno until he gets back ;) |
|
Also, @pat270
|
As requested here:
liferay-frontend/liferay-portal#821 (comment)
Adding Jon Mak, in order to bring balance to The Force.
|
5 participants
…class when highlighted instead of active table-active
https://issues.liferay.com/browse/LPS-127831
I was trying to figure out how to access the
rowSelectorstring inrowClassNameActive, but couldn't figure it out.Card View image card doesn't show active border because it is a special case where we can't style the card with the
input:checked ~ .cardselector or with theactiveclass on thelielement. This needs theactiveclass on the card. There was a pr that was merged on the Clay repo that addresses this. See liferay/clay#3937.