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

Conversation

joelamb
Copy link
Contributor

@joelamb joelamb commented Sep 25, 2023

This is a temporary measure to ensure column width preferences are persisted with a key that can be used to retrieve them.
This is only needed until issue#238 is addressed

this is a temporary measure to ensure column width preferences are persisted
with a key that can be used to retrieve them.
This is only needed until issue#238 is addressed
@changeset-bot
Copy link

changeset-bot bot commented Sep 25, 2023

🦋 Changeset detected

Latest commit: a127ffc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
ember-headless-table Patch

Not sure what this means? Click here to learn what changesets are.

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

Copy link
Contributor

@ynotdraw ynotdraw left a comment

Choose a reason for hiding this comment

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

Looks good to me - will want a changeset tho!

@@ -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 🎉

@danwenzel danwenzel removed their request for review November 27, 2023 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants