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

fix: update key for column resizing preferences #239

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/eleven-experts-worry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"ember-headless-table": patch
---

Fixes key used to save column resizing preferences.
A temporary patch ahead of fixing issue #238.
3 changes: 3 additions & 0 deletions ember-headless-table/src/-private/preferences.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ class TrackedPreferences {
return [...this.plugins.values()].every((pluginPrefs) => pluginPrefs.isAtDefault);
}

/**
* @param {string} name - this needs to be provided as klass.name, eg ColumnResizing.name
*/
forPlugin(name: string) {
let existing = this.plugins.get(name);

Expand Down
2 changes: 1 addition & 1 deletion ember-headless-table/src/plugins/column-resizing/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ export class TableMeta {
let tablePrefs = this.table.preferences;

for (let column of visibleColumnMetas) {
let existing = tablePrefs.storage.forPlugin('ColumnResizing');
let existing = tablePrefs.storage.forPlugin(ColumnResizing.name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on #238 (comment), it sounds like maybe we shouldn't be doing this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If anything just the class should be passed, not a string, nor a property on the string.
It would be the responsibility of forPlugin to see a plugin, and grab the key off of it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ynotdraw Agreed this is a temporary fix until we fix the underlying issue with preference keys, for which I've had a go at fixing here: #237

Copy link
Contributor

@ynotdraw ynotdraw Sep 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I'm wondering now based on what @NullVoxPopuli says if this won't even be a temporary fix?

Copy link
Contributor Author

@joelamb joelamb Sep 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NullVoxPopuli do you mean something like:

forPlugin(klass: PluginClass<any>) {
    let instance = Reflect.construct(klass);
    let existing = this.plugins.get(instance.name);
    if (!existing) {
      existing = new TrackedPluginPrefs();
      this.plugins.set(instance.name, existing);
    }
    return existing;
  }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, the class-references are primary public API 🎉

let columnPrefs = existing.forColumn(column.key);

columnPrefs.set('width', column.width.toString());
Expand Down