List View: Expand rows when clicking anywhere in the row. #400
Conversation
|
I have found that leaving the expanding alone for the expansion indicator and using the existing itemClick method to toggle expansion does not require the prevention of the event propagation. This also would allow for some logic around handling row expansion and row selection options. I'm not sure what the right way to handle it is, but when using row selection and row expansion what should happen when the row is clicked? Double clicked? I'm OK with your solution if we want to expand always regardless of the selection options, however you would also need to stop event propagation when the kebab menu items are clicked (in the handleMenuAction function). |
|
Hi @jeff-phillips-18 , you are right. Placing event.stopPropagation() on everywhere is not a best preactice. |
|
@jeff-phillips-18 ， @bleathem ， could you give me new suggestions ? |
|
This change looks good to me. WDYT @jeff-phillips-18? |
|
My only concern is any applications currently handling the click would get both the expansion and the click handling for the row. I suppose there should be no reason to have both. I'm OK with this change but let's be sure to release note it so any applications currently handling the click are aware. |
|
Thanks @jeff-phillips-18. I'd like to work out what the release notes should say before we merge this PR. If I understand correctly, your concern is that if useExpandingRows is enabled, and the application has implemented it's own click listener, than the applications click listener would not be able to prevent the row expansion? |
|
Correct. Some applications may have already added a click listener to toggle expansion. Adding this would then toggle twice resulting in nothing more than a flicker. I'm not sure there is much reason to have a click listener on the row when using expanding rows if we are expanding the rows on a click. This solution would certainly suggest that an application handling the click be unnecessary and discouraged when using expanding rows. So the question becomes, do we continue to call the onClick listener when using expanding rows? |
|
I think continuing to invoke the onclick listener makes sense, as we can't anticipate all the use cases an application might have for listening to that click event. In fact, perhaps we should invoke the onclick listener first, prior to expanding the rows, to give the application a chance to prevent the automatic row expansion. WDYT @jeff-phillips-18? |
|
Interesting, we could do that. We would have to document that the onClick function would need to return false prevent further handling of the event (expansion). |
|
+1, I think it's worth pursuing. Would we set the continueEvent variable to the return value of the click listener? |
|
Hi @jeff-phillips-18 @bleathem , I got your points, but I have a question -- If a user enable "useExpandingRows" why does he still want to write expand/collapse effect on clicking list item by himself. |
|
Maybe he is just thinking the expand/collapse effect of angular-patternfly is not cool enough. Then he just need to disable "useExpandingRows" and add his own logic for expand/collapse effect into the custom onClick() function. |
|
Additionally, I think it's not friendly to enforce user to append any statements(including simple |
|
The issue is, this was not there before. So, applications may be doing something different on a row click (navigate, drill down, or expand themselves). Adding this will cause unexpected results for those applications (a quick expansion of the row prior to navigation or a double toggle of expansion). We can leave this as is and release note it but if we don't allow the application to prevent the expansion, then the application's behavior may change. Adding a return is optional, only if the application returned false would the expansion not occur. |
|
Great. @jeff-phillips-18 , @bleathem , How could we handle the expansion if user has inserted async operation into onClick function ? |
|
If we want to handle the async case, we can check if the user returns a promise. Then promise would then resolve to true/false which would enable/disable expansion. I think that would be overkill at this point, let's leave it as a sync operation, and implement the async use case if users request it. Agree with @jeff-phillips-18. Returning false is optional. It allows the user to take over expanding rows if they choose, but the default behaviour works as expected. As for a use case where a user wants expanding rows, but wants to handle it themselves is if they want to conditionally allow for expansion. eg. maybe you can only expand an item if you have first checked an "agree to terms" check box (as an arbitrary example), or one could consider role-based conditionals. |
|
Thanks for updating the PR @dabeng. However can we please break apart that compound conditional to improve the readability of the code? I recommend storing the return value of the |
| enableRowExpansion = scope.config && scope.config.useExpandingRows && item && !item.disableRowExpansion; | ||
| if (typeof clickReturnValue === 'undefined' && enableRowExpansion) { | ||
| scope.toggleItemExpansion(item); | ||
| } |
There was a problem hiding this comment.
If there is no onClick function in the config, then expansion should happen.
ie:
if (!angular.isFunction(scope.config.onClick) || scope.config.onClick(item, e) !== false) {
...
}
|
Thanks @dabeng. @jeff-phillips-18 are you happy to see this one merged? |
|
there should be an update to the ngDocs for this, documenting the change to the onClick function's return value use. Also, please add this documentation to this PR's description and to the commit so that it can be easily added to the release notes. |
Note: row expansion is the default behavior after onClick performed, but user can stop such default behavior by adding the sentence "return false;" to the end of onClick function body
|
Hi @jeff-phillips-18 , I have aded additional explanations both on ngDocs and PR description |
jeff-phillips-18
changed the title
|
Please make sure the fix is merged into angular-patternfly v4 as well. |
For list views using row expansion, the row will now expand when the user clicks anywhere on the row. Previously the user had to click on the expansion indicator icon. If desired, the application code can prevent the expansion on click of the row by returning false from the config.onClick function.