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

Traits of type "color" do not work #731

Closed
ryandeba opened this issue Jan 9, 2018 · 2 comments
Closed

Traits of type "color" do not work #731

ryandeba opened this issue Jan 9, 2018 · 2 comments
Labels

Comments

@ryandeba
Copy link
Contributor

ryandeba commented Jan 9, 2018

Hi @artf,

I had previously mentioned in a pull request that traits of type color do not work. At the time I didn't have a need for them, but now I do so I'd like to look into fixing it if you haven't already started on it. The bug is in the InputColor object when it calls model.setValueFromInput - traits don't have that method so it throws an error. What do you think is the best way to fix this? Does the Trait object need a setValueFromInput method? Or should InputColor change so that it's smart enough to call different methods for a Trait vs a Property? Perhaps something else?

My initial (hacky) experimentation show that changing all of the model.setValueFromInput lines in InputColor to this gets it pretty close to working...not sure if this is the right path to go down or not though:
model.setValueFromInput && model.setValueFromInput(cl, 0) || model.setTargetValue && model.setTargetValue(cl);

@artf
Copy link
Member

artf commented Jan 9, 2018

Does the Trait object need a setValueFromInput method?

This leads to the duplication of code but still think this will gonna work better

// in Trait.js
setValueFromInput(value, final = 1, opts = {}) {
	const toSet = { value };
    this.set(toSet, { ...opts, avoidStore: 1});
	
	// Have to trigger the change
	if (final) {
      this.set('value', '', opts);
      this.set(toSet, opts);
    }
  },

moreover setTargetValue it's not actually a correct way because you have to update the model first (a listener will update the target later)

The real problem is a bad design from the start. To be correct I should have inside 'abstract/ui' an InputModel and its InputView (same for other types of input), then using them inside traits and styles. Basically, the structure has to be closer to what is done with properties inside style manager but there is a little bit more work to do -.-

@lock
Copy link

lock bot commented Sep 18, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the outdated label Sep 18, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants