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

Remove heading rows downcast converter. #7601

Closed
jodator opened this issue Jul 10, 2020 · 0 comments
Closed

Remove heading rows downcast converter. #7601

jodator opened this issue Jul 10, 2020 · 0 comments
Labels
bc:minor Resolving this issue will introduce a minor breaking change. package:table

Comments

@jodator
Copy link
Contributor

jodator commented Jul 10, 2020

@scofalik & @Reinmar - I need to double check this as this changes how table is converted.

After numerous bugs with changing headings + removing rows we concluded that enough is enough. The table models sucks. Downcasting headingRows change requires moving <tr> in the view from <thead> to <tbody> or vice-versa. It can fail in multiple scenarios (#6502, #6437, #6544, #6502, #6391, #6406 + #7454) and would potentially never be OK.

Here (#7454) there was a problem with table layout post-fixer which was called after each enqueueChange() - and thus we hit limit of what API is allowing us to do. The solution is to wait with conversion and post-fixing after all table model changes are done and that table attribute converter would know that also table children are changing.

Possible considered solutions were:

  1. A "hack": set heading rows to 0, remove/add rows, set heading rows to proper value - but it lacks an API for not converting stuff at once.
  2. Some API to mark that some changes need to be either converted at once or should not trigger model post-fixers.
  3. Mark table to be re-rendered at once.

We choose option 3 using differ.refreshItem() - which is called on every headingRows attribute operation as It requires to also take external operations into account when dealing with this bug. Also the API for many TableUtils get simplified but not passing a batch instance (:tada:).


In other words the solution is to re-render whole table in the view if headingRows attribute was changed by any means.


What is needed - some kind of 👍 / 👎 - it might impact collaboration features in some way. Do we need better API for requiring re-converting whole chunk of content?

ps.: @Reinmar this is also some sort of ADR proposal (without sections - but have intro, considered steps and proposed solution).

Originally posted by @jodator in #7572

@jodator jodator added bc:minor Resolving this issue will introduce a minor breaking change. package:table labels Jul 10, 2020
@jodator jodator added this to the iteration 34 milestone Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bc:minor Resolving this issue will introduce a minor breaking change. package:table
Projects
None yet
Development

No branches or pull requests

1 participant