lion icon indicating copy to clipboard operation
lion copied to clipboard

fix: select rich to reuse LionFieldBase (no FormatMixin)

Open jorenbroekema opened this issue 3 years ago • 2 comments

What I did

  1. Create LionFieldBase that doesn't include FormatMixin
  2. Make select rich use LionFieldBase to share a common ancestor with other fields

It seems that the order of FormatMixin is important, especially in the connectedCallback, and by abstracting Field as FieldBase (excludes the FormatMixin, which should not be applied to listbox), you no longer control the order of the FormatMixin in Field relative to the mixins in FieldBase.

I managed to fix most broken tests by putting a piece of the FormatMixin connectedCallback above the super, so it executes earlier than e.g. the connectedCallback of IStateMixin. This was important, because the sync value upwards would otherwise trigger a model-value-changed that would trigger the IStateMixin dirty setter, as it listens to this event. If we move this logic a little earlier it executes before IStateMixin has set the event listener for model-value-changed, and most tests then pass.

Except for one test...

it('reads initial value from attribute value', async () => {
  const el = /** @type {LionInput} */ (await fixture(html`<${tag} value="one"></${tag}>`));
  expect(
    /** @type {HTMLInputElement[]} */ (Array.from(el.children)).find(
      child => child.slot === 'input',
    )?.value,
  ).to.equal('one');
});

It doesn't equal 'one' but instead ''

I couldn't find a decent way to fix this yet, and I'm pretty confused on how the logic is set up to delegate the value attribute to the _inputNode value attribute.

jorenbroekema avatar May 26 '21 11:05 jorenbroekema

⚠️ No Changeset found

Latest commit: 864069d02040bc5eb1c37c39f7056bce9a077ae6

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

changeset-bot[bot] avatar May 26 '21 11:05 changeset-bot[bot]

This PR still needs some attention, maybe we can bring it back to live and finalise it soon! Anybody lending a hand, feel free 💪!

gerjanvangeest avatar Oct 04 '23 14:10 gerjanvangeest