Skip to content

Conversation

@MayaKirova
Copy link
Contributor

Closes #5288

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

@3phase 3phase added 💥 status: in-test PRs currently being tested ✅ status: verified Applies to PRs that have passed manual verification and removed ❌ status: awaiting-test PRs awaiting manual verification 💥 status: in-test PRs currently being tested labels Jul 18, 2019
@ChronosSF ChronosSF requested a review from rkaraivanov July 23, 2019 12:52
ChronosSF
ChronosSF previously approved these changes Jul 23, 2019
@ChronosSF ChronosSF added 💥 status: in-test PRs currently being tested and removed ✅ status: verified Applies to PRs that have passed manual verification labels Jul 23, 2019
@ChronosSF ChronosSF self-assigned this Jul 23, 2019
@ChronosSF
Copy link
Member

ChronosSF commented Jul 23, 2019

I tested each sample in the demos repo with width null (8.0 branch as it includes all features that have to work with this). These are my findings:

  • most (maybe all but I noticed it half-way through so I am not 100% sure) throw similar to the following error:

ERROR Error: ExpressionChangedAfterItHasBeenCheckedError: Expression has changed after it was checked. Previous value: 'width: 38843px'. Current value: 'width: 38859px'.
at viewDebugError (vendor.js:61388)
at expressionChangedAfterItHasBeenCheckedError (vendor.js:61376)
at checkBindingNoChanges (vendor.js:61576)
at checkNoChangesNodeInline (vendor.js:71156)
at checkNoChangesNode (vendor.js:71143)
at debugCheckNoChangesNode (vendor.js:71747)
at debugCheckRenderNodeFn (vendor.js:71701)
at Object.eval [as updateRenderer] (GridPerformanceSampleComponent.ngfactory.js:237)
at Object.debugUpdateRenderer [as updateRenderer] (vendor.js:71690)
at checkNoChangesView (vendor.js:71044)

  • hidden columns are incorrectly included in the total width
  • gridCellEditing - the only column (the first one that's templated with buttons) which has width unset assigns a very weird 414px width.
  • the column groups sample looks quite broken
  • moving sample - window doesn't scroll with the dragged column header (that's expected, I am not arguing that but weirdly enough the multi-cell selection works, maybe we should support it here as well)
  • mrl sample - it seems the mrl grid cuts off with the window, not with the columns
  • mrl generator sample - think the column don't have width assings which just destroys the grid
  • general navigation - any navigated element out of view is just not followed by the scroll - if we are to support width=null we'll need new navigation code for this
  • percent width columns sample - with % columns the columns are rendered in a very weird (small) size but not 0 (now I don't expect this to work generally speaking, but it will be nice to document it)
  • remote sample - grid renders without width initially which makes sense but we might need to have a min width to render the Empty Data message first? or at least document this
  • row dragging sample - broken (extra space)
  • search sample - searching on the column that's out of view doesn't scroll to it (similar to navigation)
  • hierarchical grid sample
    • first grid - null width in child layout super breaks it
    • third sample - you can't navigate to the cells to the right in child layouts - this is similar to other navigation issues but I feel this might be harder to fix for hgrid
  • hierarchical grid updating sample - the third layout is broken. This seems to happen when there is no data even if columns are assigned (but not with width) - the content then gets squashed into a 1px width or so.

@ChronosSF ChronosSF added 🛠️ status: in-development Issues and PRs with active development on them and removed 💥 status: in-test PRs currently being tested labels Jul 23, 2019
@ChronosSF ChronosSF assigned MayaKirova and unassigned ChronosSF and 3phase Jul 23, 2019
@ChronosSF ChronosSF added 💥 status: in-test PRs currently being tested and removed ❌ status: awaiting-test PRs awaiting manual verification labels Jul 31, 2019
@ChronosSF ChronosSF assigned ChronosSF and unassigned MayaKirova Jul 31, 2019
@skrustev
Copy link
Member

skrustev commented Jul 31, 2019

For a grid with width null and columns width set to percentages, the columns are sized afterwards based on the initially calculated grid size. The initial grid size is calculated on the basis that the columns have width 136px

Steps to reproduce:

  1. Open the 'Grid percentage' demo
  2. Set all columns width to '30%', unpinned and the grid width to 'null'.
  3. Observe the size of the columns.

Their combined width is bigger than the calculated grid size and they go out of the grid view area.

@ChronosSF ChronosSF added 🛠️ status: in-development Issues and PRs with active development on them and removed 💥 status: in-test PRs currently being tested labels Jul 31, 2019
@ChronosSF ChronosSF assigned MayaKirova and unassigned ChronosSF Jul 31, 2019
@ChronosSF
Copy link
Member

Check the new issues in #5397

@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 1, 2019
@ChronosSF ChronosSF added ✅ status: verified Applies to PRs that have passed manual verification and removed ❌ status: awaiting-test PRs awaiting manual verification labels Aug 2, 2019
@mpavlinov mpavlinov added the squash-merge Merge PR with "Squash and Merge" option label Aug 5, 2019
@mpavlinov mpavlinov merged commit 23dc339 into 7.3.x Aug 5, 2019
@mpavlinov mpavlinov deleted the mkirova/fix-5288-7.3.x branch August 5, 2019 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

grid: general squash-merge Merge PR with "Squash and Merge" option version: 7.3.x ✅ status: verified Applies to PRs that have passed manual verification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants