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.

Sign up for GitHub

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

Jump to bottom

Implement Row Pinning for igxHierarchicalGrid. #6979

Merged
merged 36 commits into from
Apr 13, 2020
Merged

Implement Row Pinning for igxHierarchicalGrid. #6979

merged 36 commits into from
Apr 13, 2020

Conversation

skrustev
Copy link
Member

@skrustev skrustev commented Mar 25, 2020 •
edited by ChronosSF
Loading

Related #6640

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code
  • This PR includes API docs for newly added methods/properties
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes
  • This PR includes behavioral changes and the feature specification has been updated with them

@skrustev skrustev changed the title Initial implementation of row pinning for igxHierarchicalGrid. Implement Row Pinning for igxHierarchicalGrid. Mar 25, 2020
MayaKirova
MayaKirova requested changes Mar 27, 2020
View reviewed changes
Copy link
Contributor

@MayaKirova MayaKirova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we decided not to add an additional row type are the changes in navigation services still necessary? If not could you remove them as they will be outdated anyway when the kb navigation enhancements are merged:
#6910

MayaKirova
MayaKirova previously requested changes Mar 27, 2020
View reviewed changes
projects/igniteui-angular/src/lib/grids/hierarchical-grid/hierarchical-cell.component.html Outdated
@@ -0,0 +1,31 @@
<ng-template #defaultCell>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a need for a new hierarchical-cell template? As the same checks for the displayPinnedChip and the same template for the additional chip will probably need to be added to tree grid cell, perhaps it will be best to have this in the base cell so it can be re-used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is supported by both but the regular cells do not support it, so it will only pollute their template. Also the tree grid cell has a separate cell template so it will need to be added there anyway.

projects/igniteui-angular/src/lib/grids/hierarchical-grid/hierarchical-grid.component.html Outdated Show resolved Hide resolved
projects/igniteui-angular/src/lib/grids/hierarchical-grid/hierarchical-grid.component.html Outdated
@@ -102,7 +124,7 @@
[igxForContainerSize]='calcHeight' [igxForItemSize]="renderedRowHeight" [igxForTrackBy]='trackChanges'
#verticalScrollContainer (onChunkPreload)="dataLoading($event)">
<ng-template #hierarchical_record_template>
<igx-hierarchical-grid-row [gridID]="id" [index]="rowIndex" [rowData]="rowData" #row>
<igx-hierarchical-grid-row [gridID]="id" [index]="getRowIndex(rowIndex, false)" [ghostRow]="isGhostRecord(rowData)" [rowData]="!isGhostRecord(rowData) ? rowData : rowData.recordData" #row>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the name of the property - ghostRow. I'm not sure whether we should call it something generic like that as it shows pinning specific UI (pinned chip in the first cell) and is something unique to the pinning feature. Perhaps something like 'pinPlaceholder' would make its purpose more obvious. Same for related apis - isGhostRecord, ghostRec etc.

Copy link
Member Author

@skrustev skrustev Mar 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I named it like that because the ghost thing on its own, the way I implemented does not know about the pinning. The pinned chip should be shown only when the row returns true if it is pinned as well, not only being a ghost row. I am ok to name it something else if you think this should not be the case and should be tied only to pinning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussing it further with the team we have agreed that we'll have a disabled input in the base grid row, which will apply disabled styles and prevent some user interactions when set to true (selection and editing). Pinning will utilize the disabled input and duplicated row in the unpinned area will have it set to true . If a row is both disabled and pinned it will also render the "Pinned" chip in the first cell.

@dkamburov
Copy link
Contributor

I encountered a bit weird behavior. I'm I missing something here? The second pinned row disappeared when scrolling and then the children seems to have bigger z-index as they are showing on top of pinned area:
hgridpin

@skrustev skrustev force-pushed the skrastev/hgrid-row-pinning branch from d41097a to 57c5955 Compare March 31, 2020 12:29
@ChronosSF ChronosSF requested a review from MayaKirova April 1, 2020 07:12
@MayaKirova MayaKirova added 💥 status: in-test PRs currently being tested and removed ❌ status: awaiting-test PRs awaiting manual verification labels Apr 1, 2020
@skrustev skrustev marked this pull request as ready for review April 1, 2020 10:24
@MayaKirova
Copy link
Contributor

@skrustev If the first column is templates, then the pinned chip is not rendered anywhere in the pinned row, hence it gives no indication that the row is pinned. The rows looks just disabled.
The pinned chip should behave like the tree grid expansion indicator - should be rendered in the first visible cell in the row, regardless of whether it is templated or not.

@ChronosSF ChronosSF self-requested a review April 9, 2020 06:53
ChronosSF
ChronosSF previously approved these changes Apr 9, 2020
View reviewed changes
@ChronosSF ChronosSF requested a review from StefanIvanov April 9, 2020 11:06
@ChronosSF ChronosSF added ✅ status: verified Applies to PRs that have passed manual verification and removed 🛠️ status: in-development Issues and PRs with active development on them labels Apr 10, 2020
@radomirchev radomirchev mentioned this pull request Apr 10, 2020
Closed
30 tasks
@skrustev skrustev requested a review from simeonoff April 10, 2020 14:18
@ChronosSF ChronosSF self-requested a review April 10, 2020 14:18
ChronosSF
ChronosSF previously approved these changes Apr 10, 2020
View reviewed changes
StefanIvanov
StefanIvanov previously approved these changes Apr 10, 2020
View reviewed changes
@skrustev skrustev dismissed stale reviews from StefanIvanov and ChronosSF via e837fb3 April 10, 2020 14:55
@ChronosSF ChronosSF self-requested a review April 10, 2020 15:08
ChronosSF
ChronosSF previously approved these changes Apr 10, 2020
View reviewed changes
ChronosSF
ChronosSF previously approved these changes Apr 13, 2020
View reviewed changes
@ChronosSF ChronosSF self-requested a review April 13, 2020 16:59
@ChronosSF ChronosSF merged commit 238d326 into master Apr 13, 2020
@ChronosSF ChronosSF deleted the skrastev/hgrid-row-pinning branch April 13, 2020 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChronosSF ChronosSF ChronosSF approved these changes

@StefanIvanov StefanIvanov StefanIvanov left review comments

@MayaKirova MayaKirova Awaiting requested review from MayaKirova

@damyanpetev damyanpetev Awaiting requested review from damyanpetev

@rkaraivanov rkaraivanov Awaiting requested review from rkaraivanov

@simeonoff simeonoff Awaiting requested review from simeonoff

Assignees

@dkamburov dkamburov

@ChronosSF ChronosSF

@VMihalkov VMihalkov

Labels
grid: hierarchical-grid grid: row-pinning ✅ status: verified Applies to PRs that have passed manual verification
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@skrustev @dkamburov @MayaKirova @ChronosSF @StefanIvanov @simeonoff @VMihalkov @kdinev