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

Textfield to support dependant values #445

Closed
wants to merge 3 commits into from

Conversation

drehelis
Copy link
Contributor

Following my embarrassing issue #442, trying to cherry-pick this change to hopefully merge it to master.

I'm not entirely sure the implementation is right thought. Meanwhile, this is what I got -

Screen.Recording.2021-05-25.at.23.16.55.mov

@bugy
Copy link
Owner

bugy commented May 26, 2021

Hi @autogun, thanks for the PR! I'll check it later

One question: what happens, if you change value in "Drop-A"? Will "Text-A" be updated?

But to be honest, I would prefer to go the way of #320 here, i.e. use "default" value rather than "values". For me, "default" option feels more natural.

@drehelis
Copy link
Contributor Author

Yes, Text-A is updated accordingly to the Drop-A selection.

By use default, you mean from the configuration point of view?
To change this:

    {
      "name": "Text-A",
      "values": {
        "script": "echo ${Drop-A}_"
      },
      "required": true
    },

to:

    {
      "name": "Text-A",
      "default": {
        "script": "echo ${Drop-A}_"
      },
      "required": true
    },

@bugy
Copy link
Owner

bugy commented May 26, 2021

Yes, exactly this one

@drehelis
Copy link
Contributor Author

How about consistency? Across all config, default is key:value and not an object.
While values are all objects with parameter injection ability.

@bugy
Copy link
Owner

bugy commented May 26, 2021

Apparently, script option is already supported there: #141 😅
But it doesn't support dependant parameters yet

As for me, the word default reflects the behaviour better, than values

@drehelis
Copy link
Contributor Author

@bugy, one way to achieve this is in my last commit, but that still uses the infrastructure of values.
This, however, allows me to use both default and values in a text config.

@bugy
Copy link
Owner

bugy commented Jun 6, 2021

Hi @autogun, sorry for taking so long. To be honest, I would prefer to keep the code explicit, in this sense I would suggest to introduce explicit default_value_provider, which would set a single default value.
Then you would need to extend model.parameter_config.ParameterModel._param_values_observer to work with the default values provider
And create a method similar to model.parameter_config.ParameterModel._reload_values, which would set default

On the client side, you would need to listen to "default" changes (not values)
When an old default value still equals to this.value, then you can update it, by calling the following code:

      this._doValidation(newValue);
      this.$emit('input', newValue);

@bugy
Copy link
Owner

bugy commented Mar 16, 2023

supported via "default" option in scope of #320. So closing this one as obsolete.

@bugy bugy closed this Mar 16, 2023
@drehelis drehelis deleted the dependant_text branch March 16, 2023 14:57
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.

2 participants