Simplify working with columns #40
Conversation
NullVoxPopuli
force-pushed
the
simplify-working-with-columns
branch
3 times, most recently
from
b8ca44b to
31e70e0
Compare
Preview URLsGH Env: preview |
NullVoxPopuli
force-pushed
the
simplify-working-with-columns
branch
2 times, most recently
from
adc2e7a to
98b31af
Compare
| * This works recursively up the plugin tree up until a plugin has no requirements, and then | ||
| * all colums from the table are returned. | ||
| */ | ||
| function columnsFor<DataType = any>( |
There was a problem hiding this comment.
this is the code that powers all the refactors in this PR.
It's also some of the messier code.
I wasn't really in a headspace to do recursive trees, so this is what works for now.
If we add another plugin that needs to tweak columns rendered, then we'll have to do this proper
| @@ -45,6 +44,10 @@ export class ColumnReordering extends BasePlugin<Signature> { | |||
|
|
|||
| tableMeta.reset(); | |||
| } | |||
|
|
|||
| get columns() { | |||
There was a problem hiding this comment.
new API on plugins -- allows overriding of what columns should be rendered
There is now a new "columns" query-API that "does the right thing", based on a "requester" (plugin class). There is a hierarchy of column behaviors that can be statically known, and the resolution of columns allows plugins to be independent of other plugins, while also layering in the correct order, based on if the plugin defines a columns property.
NullVoxPopuli
force-pushed
the
simplify-working-with-columns
branch
from
98b31af to
4d45e14
Compare
NullVoxPopuli
requested review from
vitch,
ynotdraw,
camskene,
danwenzel and
nicolechung
NullVoxPopuli
force-pushed
the
simplify-working-with-columns
branch
from
4d45e14 to
355e4ab
Compare
| import { meta } from 'ember-headless-table/plugins'; | ||
| import { ColumnVisibility } from 'ember-headless-table/plugins/column-visibility'; | ||
| import { meta, columns } from 'ember-headless-table/plugins'; | ||
| import { ColumnVisibility, hide, show } from 'ember-headless-table/plugins/column-visibility'; |
There was a problem hiding this comment.
Nice, I like how these methods (hide & show) are exposed (same with above plugin tests) - this is really clean!
…(which is defined earlier in the file)
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
| assert(`First argument passed to columns.for must be an instance of Table`, table[TABLE_KEY]); | ||
|
|
||
| let visibility = findPlugin(table.plugins, 'columnVisibility'); | ||
| let reordering = findPlugin(table.plugins, 'columnOrder'); |
There was a problem hiding this comment.
Trying to work on #152 and stumbled across this after some digging...
Is this how the column order plugin ends up getting the columns with visibility already applied?
There was a problem hiding this comment.
Yeah, there is a graph of plugins determined at lookup time. You can choose:
- get all columns
- get columns for the plugin (subject to the graph)
- get user land columns (after all plugins are applied)
There was a problem hiding this comment.
I get that the column order plugin needs to be aware of individual column's visibility values but I think it still needs to be able to loop over all columns...
Looking at those things you can choose, after a bunch of clicking through the code I think the APIs are:
- get all columns:
table.columns(assuming you have a reference totable, otherwise you'll need to thread it down - get columns for the plugin (subject to the graph):
columns.for(table, PluginClass)(again, you need a reference to thetable - get user land columns (after all plugins are applied): I haven't needed this one yet so I haven't found it...
Does that seem right?
This was a feature request / feedback from @vitch a while back when we were first open sourcing this library.
Motivation
Prior to this PR, working with columns from the plugins in a hierarchical way leaked the hierarchy between the plugins, preventing consistent use of columns APIs for the consumer -- a plugin author would need to know the hierarchy, and know which plugin to access to get the correct columns.
This has now all bee abstracted away in a new set of queries helpers called
columns.Requires: #12
implement "optionally requires" to imply a hierarchy, as hard-requiring another plugin to be used "should be" rarecan be done in a later PR, when the need arisescolumnsForPlugin(table, requestingPlugin)gets the columns appropriate for the requesting plugin - automatically figures out plugin hierarchy
columnsForPlugin(but that's an implementation detail)nextColumn(focusedColumn, requestingPlugin)previousColumn(focusedColumn, requestingPlugin)columnsAfter(column, requestingPlugin)columnsBefore(column, requestingPlugin)ColumnResizingStickyColumnsColumnReorderingColumnResizingFor review
First, you'll want to look at the tests, in particular for ColumnReordering and ColumnResizing -- these are refactors that were enabled after all the above was implemented.
Then, you'll want to read through the changes to plugin/-private/base.ts, and then see how those changes had an effect on each plugin.
I'm hesitant to update the demos to match the tests now, because the non-gjs ergonomics are really sub-par, and I need to add gjs/gts support to Docfy. I have minimally updated the demos that were directly accessing plugins' Meta to get the columns tho.