Skip to content

Conversation

@MayaKirova
Copy link
Contributor

@MayaKirova MayaKirova commented Aug 21, 2019

Closes #5621
Closes #5540
Closes #4530

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 added 🛠️ status: in-development Issues and PRs with active development on them and removed ❌ status: awaiting-test PRs awaiting manual verification labels Aug 21, 2019
@MayaKirova MayaKirova added ❌ status: awaiting-test PRs awaiting manual verification and removed 🛠️ status: in-development Issues and PRs with active development on them labels Aug 21, 2019
MKirova added 2 commits August 22, 2019 13:56
…e for header for HierarchicalGrid. Adding expandAll/collapseAll api as defined in spec.
ChronosSF
ChronosSF previously approved these changes Aug 22, 2019
MKirova added 3 commits August 22, 2019 17:54
…nt state (igxRowExpandedIndicator/igxRowCollapsedIndicator and igxHeaderExpandedIndicator/igxHeaderCollapsedIndicator). Updating sample with custom icons.
@3phase 3phase removed the 💥 status: in-test PRs currently being tested label Aug 30, 2019
@mpavlinov mpavlinov added 🛠️ status: in-development Issues and PRs with active development on them and removed ✅ status: verified Applies to PRs that have passed manual verification labels Aug 30, 2019
@mpavlinov
Copy link
Contributor

@MayaKirova Here is the feedback:

  1. Toggle action logic stays above the template - template will be used only for visual changes
  2. Expose an input for field name which will indicate whether to hide/show the expand/collapse

@MayaKirova
Copy link
Contributor Author

MayaKirova commented Sep 3, 2019

@MayaKirova Here is the feedback:

  1. Toggle action logic stays above the template - template will be used only for visual changes
  2. Expose an input for field name which will indicate whether to hide/show the expand/collapse

@mpavlinov
Regarding 1) -> Does this include the header expand/collapse template or just the ones for the data rows?
Regarding 2) -> Should this be an input on the hierarchical grid (one for all levels of the hierarchy) or should it also be settable per rowIsland? Additionally, should this prevent expansion triggered from outside the template (for example, expandAll Api on the grid, toggle api called on the row instance etc.)

@mpavlinov
Copy link
Contributor

mpavlinov commented Sep 3, 2019

@MayaKirova Here is the feedback:

  1. Toggle action logic stays above the template - template will be used only for visual changes
  2. Expose an input for field name which will indicate whether to hide/show the expand/collapse

@mpavlinov
Regarding 1) -> Does this include the header expand/collapse template or just the ones for the data rows?
Regarding 2) -> Should this be an input on the hierarchical grid (one for all levels of the hierarchy) or should it also be settable per rowIsland? Additionally, should this prevent expansion triggered from outside the template (for example, expandAll Api on the grid, toggle api called on the row instance etc.)

  1. Only for the data rows. See Expand Row Indicator and Collapse Row Indicator sections in spec.
  2. For hierarchical grid and for row islands. Row island will override the hierarchical grid one. Yes. It should prevent expansion from outside the template. After all the developer says that there are not children. See hasChildrenKey in spec.

…ate. Template can not be used to change default behavior.
@MayaKirova
Copy link
Contributor Author

@kdinev Please confirm that the spec complies with your expectations so that we can continue with the implementation based on the new requirements.

@kdinev
Copy link
Member

kdinev commented Sep 4, 2019

@MayaKirova I'm wondering why in the example the hgrid instance has to be passed to the function call from that hgrid instance?

<igx-icon role="button" fontSet="material" (click)="hgrid.expandAll(hgrid)">add</igx-icon>

I'm also starting to lead towards exposing both expand all and collapse all templates out of the box, with an hgrid input for whether the expandAll template should be shown, defaulting to false. This way the user would be able to show expandAll with our default template by simply setting [showExpandAll]="true". Then, if they want to change the icon, they would pass an expand all template, but wouldn't have to call the expand all grid logic from the template element.

@MayaKirova
Copy link
Contributor Author

@kdinev

hgrid doesn’t need to be passed as the expandAll api takes no params. Seems to be a problem in the spec snippet.

Reg 2) Yes. It’s currently a bit inconsistent since the header template allows changing the behavior (since api calls are done inside the template) while the ones for the rows do not.
I’m ok with adding an input and documenting as a limitation that expand all operation will have a negative performance impact as it will create a large number of child hierarchical grids.

Let me know if that’s what we’ll go with and I’ll update the spec accordingly.

@MayaKirova
Copy link
Contributor Author

@kdinev
I have updated the Expand All Header Indicator section of the spec and added information on the new hierarchical grid properties.
Please review the spec and let me know if this will be the final approach we should implement.
In short:

  • Templates (both row and header) will serve only visualization purpose ( cannot change default behavior via the templates.)
  • To achieve feature request Allow to expand all rows in hierach grid #5540 (Allow to expand all rows in hierach. Grid) we will add an additional input – showExpandAll, which when set will render the expand all template and users will be able to expand all rows in the current grid via that template.
  • To achieve feature request Hide expansion indicator in hierarchical grid if no data found #4530 (Hide expansion indicator in hierarchical grid if no data found) we will add an additional input – hasChildrenKey, which when set will check for a property(boolean) in the data source record, that indicates whether there are children for the particular record. If record does not have children (according to the prop value in the record) it will not show expand icon and will not be expandable (cannot be toggled via expand/collapse all api or other api calls).

@kdinev
Copy link
Member

kdinev commented Sep 4, 2019

@MayaKirova I like it! Go ahead and make the necessary changes.

@MayaKirova MayaKirova added ❌ status: awaiting-test PRs awaiting manual verification and removed 🛠️ status: in-development Issues and PRs with active development on them labels Sep 5, 2019
@MayaKirova
Copy link
Contributor Author

@3phase Please re-retest according to the agreed on spec below:

@kdinev
I have updated the Expand All Header Indicator section of the spec and added information on the new hierarchical grid properties.
Please review the spec and let me know if this will be the final approach we should implement.
In short:

  • Templates (both row and header) will serve only visualization purpose ( cannot change default behavior via the templates.)
  • To achieve feature request Allow to expand all rows in hierach grid #5540 (Allow to expand all rows in hierach. Grid) we will add an additional input – showExpandAll, which when set will render the expand all template and users will be able to expand all rows in the current grid via that template.
  • To achieve feature request Hide expansion indicator in hierarchical grid if no data found #4530 (Hide expansion indicator in hierarchical grid if no data found) we will add an additional input – hasChildrenKey, which when set will check for a property(boolean) in the data source record, that indicates whether there are children for the particular record. If record does not have children (according to the prop value in the record) it will not show expand icon and will not be expandable (cannot be toggled via expand/collapse all api or other api calls).

@3phase 3phase added 💥 status: in-test PRs currently being tested and removed ❌ status: awaiting-test PRs awaiting manual verification labels Sep 9, 2019
@3phase 3phase added ✅ status: verified Applies to PRs that have passed manual verification and removed 💥 status: in-test PRs currently being tested labels Sep 9, 2019
@zdrawku zdrawku merged commit bcc5ae0 into master Sep 9, 2019
@zdrawku zdrawku deleted the mkirova/fix-5621 branch September 9, 2019 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

10 participants