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

feat(rowDetail): add override method to know which row is expandable #351

Merged
merged 2 commits into from
Mar 11, 2019

Conversation

ghiscoding
Copy link
Collaborator

@ghiscoding ghiscoding commented Mar 6, 2019

Similar to what we did with PR #346 on Row Selection with an override method, let's add the same feature to the Row Detail Expandable. The override method is named expandableOverride and follows the exact same concept as the PR #346

Example

// make only every 2nd row an expandable row,
// by using the override function to provide custom logic of which row is expandale
detailView.expandableOverride(function(row, dataContext, grid) {
   return (dataContext.id % 2 === 1);
});

ujmoeolar6

@ghiscoding
Copy link
Collaborator Author

@SatanEnglish
If you have time to look into the changes and let me know if I missed anything, basically it's the same concept as what we did with PR #346 but for Row Detail.

Also I'd like to know, there seems to be a min-height that is unnecessary and is causing the detail panel to always have a scroll. Basically this line, I'd like to remove the min-height from the div.
Please note, there are 2 min-height set in the plugin, I believe the main container still needs it to work correctly but the one I want to remove is unnecessary, so please make sure you only remove this line when you test.

- html.push('<div class="detail-container detailViewContainer_', dataContext.id, '" style="min-height:' + dataContext[_keyPrefix + 'height'] + 'px">');
+ html.push('<div class="detail-container detailViewContainer_', dataContext.id, '">');

With this change, the visual difference is shown below.

BEFORE
row_detail_before

AFTER FIX
row_detail_after

Can I push this fix (min-height), can you try it on your side first?

@ghiscoding ghiscoding added the wip label Mar 6, 2019
@SatanEnglish
Copy link
Contributor

SatanEnglish commented Mar 6, 2019

@ghiscoding I'll try have a play tomorrow.
Have a feeling the min-height is needed however I'll double check.

PS: Looks like I may have missed the 25px that are for the offset from min calculation.

<div class="dynamic-cell-detail cellDetailView_3" style="height:275px; top:25px">

// This should take into account 25px from the div above top25px
<div class="detail-container detailViewContainer_3" style="min-height:275px">

Anyway I'll have a look at work.

@ghiscoding
Copy link
Collaborator Author

To stick with certain naming conventions, as mentioned in this comment, I've renamed the override method to expandableOverride

@SatanEnglish
Copy link
Contributor

@ghiscoding
I've tested my pages looks like your right you can remove.

- html.push('<div class="detail-container detailViewContainer_', dataContext.id, '" style="min-height:' + dataContext[_keyPrefix + 'height'] + 'px">');
+ html.push('<div class="detail-container detailViewContainer_', dataContext.id, '">');

PS: Sorry took so long to check

@ghiscoding
Copy link
Collaborator Author

@SatanEnglish
Thanks for checking that out. Are you ok with the override before I merge this?

@SatanEnglish
Copy link
Contributor

@ghiscoding Don't like the name as it doesn't explain itself.
BUT it's named that for consistency in which case yes I'm happy with override from what I can tell it works as expected.

Guess my only question would be is there something nicer we can show than just a blank cell.

@ghiscoding
Copy link
Collaborator Author

Nicer as what? Are you expecting something to see something else when the row is not available instead of...nothing? Personally, I prefer to see nothing instead of a disabled element which user might not understand why it's disabled.

@SatanEnglish
Copy link
Contributor

@ghiscoding All good for now.
If I decided I want it another way I'll make a pull request then and add some way to override what the disabled Item looks like. (Not 100% what this would look like right now anyway)

For now you have done a great job.

@ghiscoding ghiscoding removed the wip label Mar 11, 2019
@ghiscoding ghiscoding merged commit 116addc into master Mar 11, 2019
@ghiscoding ghiscoding deleted the feat/row-detail-override-available-rows branch May 20, 2019 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants