Skip to content

Row pinning base #6848

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

Merged
merged 41 commits into from
Mar 18, 2020
Merged

Row pinning base #6848

merged 41 commits into from
Mar 18, 2020

Conversation

MayaKirova
Copy link
Contributor

@MayaKirova MayaKirova commented Feb 28, 2020

Related to #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

@MayaKirova MayaKirova marked this pull request as ready for review March 5, 2020 13:03
@radomirchev radomirchev mentioned this pull request Mar 5, 2020
30 tasks
<span *ngIf="hasMovableColumns && draggedColumn && pinnedColumns.length <= 0"
[igxColumnMovingDrop]="headerContainer" [attr.droppable]="true" id="left"
class="igx-grid__scroll-on-drag-left"></span>
<span *ngIf="hasMovableColumns && draggedColumn && pinnedColumns.length > 0"
[igxColumnMovingDrop]="headerContainer" [attr.droppable]="true" id="left"
class="igx-grid__scroll-on-drag-pinned" [style.left.px]="pinnedWidth"></span>
<div #pinContainer *ngIf='pinnedRecords.length > 0 && isRowPinningToTop' class='igx-grid__tr--pinned igx-grid__tr--pinned-top'>
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to have both the top and bottom pinning use the same template so that there is less code repetition?

@MayaKirova
Copy link
Contributor Author

@skrustev please re-test.

ChronosSF
ChronosSF previously approved these changes Mar 13, 2020
Copy link
Member

@damyanpetev damyanpetev left a comment

Choose a reason for hiding this comment

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

We don't have noImplicitAny compilation enabled, let's not leave spots for future errors though.

* this.grid.selectedRows[0].pinned = true;
* ```
*/
public set pinned(value: boolean) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is needed, considering row.grid (and thus its API) is public + you have the shorthand pin methods.
Not a big deal though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume it was added in the specification so that it's the same as the column, although its functionality overlaps with the pin/unpin methods. @ChronosSF let me know if you still think it is necessary.

Copy link
Member

Choose a reason for hiding this comment

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

I was specifically looking at the spec for this, but I can't see anything for the row API additions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While there's no dedicated section for Row API methods, there is a mention under 3.3. Developer Experience:

"Similar to the column counterparts, the row directive has pin and unpin methods that are used under the hood, as well as pinned getter/setter for maximum flexibility to the user. "

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, still feels a bit redundant since the column is declarative and the row isn't, but one more shorthand won't hurt much.

simeonoff
simeonoff previously approved these changes Mar 16, 2020
@skrustev skrustev added ✅ status: verified Applies to PRs that have passed manual verification and removed 💥 status: in-test PRs currently being tested labels Mar 16, 2020
damyanpetev
damyanpetev previously approved these changes Mar 17, 2020
@@ -826,7 +826,11 @@ export class IgxGridComponent extends IgxGridBaseDirective implements GridType,
/**
* @hidden @internal
*/
public getContext(rowData, rowIndex): any {
public getContext(rowData: any, rowIndex: number, pinned?: boolean): any {
if (pinned && !this.isRowPinningToTop) {
Copy link
Member

Choose a reason for hiding this comment

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

@MayaKirova @ChronosSF This should definitely be noted in the feature documentation, that pinned rows will affect the start index of normal rows or are affected by the length of the data view when on the bottom.

@ChronosSF ChronosSF dismissed stale reviews from damyanpetev and simeonoff via ee478a2 March 18, 2020 09:39
@ChronosSF ChronosSF merged commit b36b75e into master Mar 18, 2020
@ChronosSF ChronosSF deleted the mkirova/row-pinning-base branch March 18, 2020 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
grid: general grid: row-pinning version: 9.1.x ✅ status: verified Applies to PRs that have passed manual verification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants