[WIP] Reordering hidden columns #185

vitch · 2023-05-17T17:36:02Z

This PR addresses #152 by tweaking the way reordering is done...

From the commit message of the last commit:

In order to make the previously added test pass we need to give
the column reordering plugin access to all columns, not just the
visible ones.
When moving left or right we need to iteratively swap the position
of adjacent columns, continuing until we have swapped with a visible
column.
There are some failing tests for internals of the component on this
commit but they can be fixed up later. I want to see if the new UX
is desirable.

There is also some implementation tweaks I'd like to make (using a
simple for loop would be more efficient because I could use the index
in the array to do the swapping) but I can do that in a separate
commit.

I'm also not sure if we could do away with or simplify the entire
swapWith function (and maybe even orderOf?). There are some
TODOs in the code as well for edge cases to clean up. At the moment
I'm looking for verification that the approach seems sound and the
generated UX is an improvement...

changeset-bot · 2023-05-17T17:36:05Z

⚠️ No Changeset found

Latest commit: 41811bb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2023-05-24T10:43:31Z

Preview URLs

GH Env: preview
docs: https://fb9a2780.ember-headless-table.pages.dev
api docs: https://fb9a2780.ember-headless-table.pages.dev/api/modules.html

As described in #152, the behaviour when reordering over hidden columns is incorrect. This test illustrates the expected behaviour so that we can fix the implementation.

…dden elements

To make it easier to play with show/hiding then moving columns

In order to make the previously added test pass we need to give the column reordering plugin access to _all_ columns, not just the visible ones. When moving left or right we need to iteratively swap the position of adjacent columns, continuing until we have swapped with a visible column. There are some failing tests for internals of the component on this commit but they can be fixed up later. I want to see if the new UX is desirable.

By adding any missing columns to the end of the list whenever the data is passed _in_ we will be able to simplify any logic around reordering because it will know that it is dealing with a full set of columns

This is the next piece of guaranteeing that we are dealing with a full set of positions within the addon code. By verifying on input we can save ourselves verifying on every calculation / output.

It turns out that we can't test this functionality because we're hitting some asserts in other functionality which should now be considered unnecessary so we'll have to do some other cleaning up before this test can go green.

vitch · 2023-07-04T10:30:44Z

test-app/tests/plugins/column-reordering/rendering-test.gts

@@ -297,11 +297,12 @@ module('Plugins | columnReordering', function (hooks) {
      assert.strictEqual(getColumnOrder(), 'A B C D');
    });

-    test('without setting the order of anything, we cannot retain the order of the columns when they are added or removed', async function (assert) {
+    test.skip('without setting the order of anything, we retain the order of the columns when they are added or removed', async function (assert) {


This test is currently skipping because it fails.

The plugin no longer supports the columns being mutated after the initial render from outside the table. Is this a legitimate thing that consumers should be able to do or was it just a simple way to initially write this test?

cc @NullVoxPopuli

Maybe, there are 3 use cases that i'm aware of:

reorder each column live / immediately (like in this repos demos)

reorder after submitting a form (setting the whole order at once) -- this is something common in table settings menus -- and relie(d|s) on a duplicate version of the ColumnOrder class (i forget the name) to have the same behavior as the columns.

same as above, but without form submit.editing in a menu immediately changes the table (a hybrid of the prior two approaches, ux wise)

In all 3 of these, order is mutated from outside the table, so maybe i'm missing something, and need to go through the code.

reorder each column live / immediately (like in this repos demos)

This works by using the plugins API, not by mutating the passed array

reorder after submitting a form

There is an API for this too (setAll IIRC)

editing in a menu immediately changes the table

I think that could use the API too...

I guess the bigger question is - should we expect consumers to use provided APIs to do things or should we let them mutate the inputs?

This works by using the plugins API, not by mutating the passed array

Ah i see, that was the key part i was missing when you said mutation -- like a TrackedArray that has had additional columns added or removed or something.

should we expect consumers to use provided APIs to do things or should we let them mutate the inputs?

I mean, one of the nice things deriving data from inputs gets us is that all of that can be handled.

Like, it could be a choice to require tearing down and recreating whatever when additional data is added to the table, but predefining all that before rendering is a common enough pattern, where it should probably be ok... If people want to asynchronously add more columns, they could probably deal with extra computation needed to re-position, re-size, etc the existing columns in addition to the new columns.

Like, personally, i'm not a fan of (what i see as) breaking reactivity (we can't know how deep or shallow a reactive abstraction is, and when we assume any depth, we can inherently any consuming usage -- now, maybe the maintainence burden in too great here, and that's fine! An assertion or something could be added to communicate that mutating the array passed to the ColumnOrder class isn't supported (tho, idk how common of a use case it is -- it's all kind of 'possibilities' here ))

So that we don't need to worry about checking for and filling consecutive slots throughout the code

johanrd · 2024-05-22T12:45:39Z

@vitch hey, great work.

I'm curious: What did you end up doing here – are you using this functionality in a fork?

vitch · 2024-05-22T20:12:44Z

@vitch hey, great work.

I'm curious: What did you end up doing here – are you using this functionality in a fork?

I'm afraid I did the work because I needed it at work and I no longer work there... @joelamb might know if there is any active work on it?

NullVoxPopuli · 2024-05-22T22:28:00Z

Would there be interest in more distributed maintenance?

repo could be transferred to https://github.com/universal-ember/ ?

vitch force-pushed the reordering-hidden-columns branch 2 times, most recently from d42a1db to e1d905b Compare May 17, 2023 22:29

joelamb force-pushed the turbo-and-sync-pnpm branch 4 times, most recently from 22b1ede to 3e471a7 Compare May 22, 2023 10:12

Base automatically changed from turbo-and-sync-pnpm to main May 22, 2023 13:34

vitch force-pushed the reordering-hidden-columns branch from 5795ecc to 20234d1 Compare May 24, 2023 10:38

github-actions bot deployed to ember-headless-table (Preview) May 24, 2023 10:42 View deployment

vitch force-pushed the reordering-hidden-columns branch from 20234d1 to 62af988 Compare May 24, 2023 11:20

github-actions bot deployed to ember-headless-table (Preview) May 24, 2023 11:24 View deployment

vitch force-pushed the reordering-hidden-columns branch from 62af988 to 0b0f086 Compare May 30, 2023 10:47

github-actions bot deployed to ember-headless-table (Preview) May 30, 2023 10:51 View deployment

vitch added 3 commits July 4, 2023 10:41

test: Add a failing test showing the reorder problem

e297c8a

As described in #152, the behaviour when reordering over hidden columns is incorrect. This test illustrates the expected behaviour so that we can fix the implementation.

test: Update test because we can and should retain the position of hi…

f3e53b3

…dden elements

demo: Add an extra column to the demo

6b6d702

To make it easier to play with show/hiding then moving columns

vitch force-pushed the reordering-hidden-columns branch from 0b0f086 to d6a1f27 Compare July 4, 2023 09:44

github-actions bot deployed to ember-headless-table (Preview) July 4, 2023 09:50 View deployment

vitch added 5 commits July 4, 2023 11:11

chore: Fix column ordering demo

e750726

feat: Explicitly add any missing columns to the end of the list

7e03df0

By adding any missing columns to the end of the list whenever the data is passed _in_ we will be able to simplify any logic around reordering because it will know that it is dealing with a full set of columns

fix: Remove any extra columns from the passed in map

68c30a5

This is the next piece of guaranteeing that we are dealing with a full set of positions within the addon code. By verifying on input we can save ourselves verifying on every calculation / output.

test: Add a test trying to hit removeExtraColumnsFromMap

3139ad6

It turns out that we can't test this functionality because we're hitting some asserts in other functionality which should now be considered unnecessary so we'll have to do some other cleaning up before this test can go green.

vitch force-pushed the reordering-hidden-columns branch from d6a1f27 to 3139ad6 Compare July 4, 2023 10:24

vitch commented Jul 4, 2023

View reviewed changes

github-actions bot deployed to ember-headless-table (Preview) July 4, 2023 10:31 View deployment

chore: Ensure consecutive zero based positions when initialised

41811bb

So that we don't need to worry about checking for and filling consecutive slots throughout the code

joelamb force-pushed the reordering-hidden-columns branch from 66919aa to 41811bb Compare July 17, 2023 11:52

github-actions bot deployed to ember-headless-table (Preview) July 17, 2023 11:56 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Reordering hidden columns #185

[WIP] Reordering hidden columns #185

vitch commented May 17, 2023

changeset-bot bot commented May 17, 2023 •

edited

github-actions bot commented May 24, 2023 •

edited

vitch Jul 4, 2023

NullVoxPopuli Jul 4, 2023

vitch Jul 4, 2023 •

edited

NullVoxPopuli Jul 4, 2023

johanrd commented May 22, 2024

vitch commented May 22, 2024

NullVoxPopuli commented May 22, 2024

[WIP] Reordering hidden columns #185

Are you sure you want to change the base?

[WIP] Reordering hidden columns #185

Conversation

vitch commented May 17, 2023

changeset-bot bot commented May 17, 2023 • edited

⚠️ No Changeset found

github-actions bot commented May 24, 2023 • edited

Preview URLs

vitch Jul 4, 2023

Choose a reason for hiding this comment

NullVoxPopuli Jul 4, 2023

Choose a reason for hiding this comment

vitch Jul 4, 2023 • edited

Choose a reason for hiding this comment

NullVoxPopuli Jul 4, 2023

Choose a reason for hiding this comment

johanrd commented May 22, 2024

vitch commented May 22, 2024

NullVoxPopuli commented May 22, 2024

changeset-bot bot commented May 17, 2023 •

edited

github-actions bot commented May 24, 2023 •

edited

vitch Jul 4, 2023 •

edited