add resolve-item callback to select-item-form-field #311
Conversation
|
Preview link: https://311.react-ui.aboutbits.dev |
| console.warn( | ||
| 'SelectItemFormField: No resolveItem function provided. This is required to resolve the selected item.', | ||
| ) |
There was a problem hiding this comment.
Should we move such console logs behind a check that the NODE_ENV is in development mode?
There was a problem hiding this comment.
If we want to force the user of this component to specify this method, then I would make it mandatory and fix it in the projects. Currently we don't force it, but annoy you with a warning message.
We should consider, that in 95% of our uses cases we don't need this function.
There was a problem hiding this comment.
Should we move such console logs behind a check that the NODE_ENV is in development mode?
I think it's not bad to always show it. Then you can see if there's a problem in production too.
We should consider, that in 95% of our uses cases we don't need this function.
That's why I would not force it. It makes it harder to use the component, and it can lead to a fake function being passed because there is no other way.
There was a problem hiding this comment.
I think it's not bad to always show it. Then you can see if there's a problem in production too.
I disagree here. In my opinion this is a log entry for a developer and therefore it should not be logged in a production environment.
That's why I would not force it. It makes it harder to use the component, and it can lead to a fake function being passed because there is no other way.
I fully agree that it makes the component harder to use. On the other hand people have to actively ignore the warning message, which will be annoying and cause discussions. I would either allow to not define the function or force it. This is also linked to the initialItem topic below, which would force you to set it all cases.
From a technical point of view always forcing the function would be the best option, because only in that case the component is fully functioning.
There was a problem hiding this comment.
I disagree here. In my opinion this is a log entry for a developer and therefore it should not be logged in a production environment.
You only see the log entry when you open the dev tools, and that's when you really want to see logs. If there are no logs, you never have a chance to see what's wrong, even in production.
On the other hand people have to actively ignore the warning message, which will be annoying cause discussions
No, you only get this warning if resolveItem is required because you are actively setting the value of the field. If you don't set it (95% of the time), you won't see this warning.
This is also linked to the initialItem topic below, which would force you to set it all cases.
That's correct, yes.
There was a problem hiding this comment.
Should we mark the initialItem property as deprecated? Do we want in the future only have the one way to resolve the information or is the initialItem concept still ok for us?
There was a problem hiding this comment.
Although it's convenient for the developer to be able to set it at the beginning if we already have the entire item, it's probably better to have just one way to resolve it. Then we can get rid of this prop and we don't even need an initialValue prop because that's handled by the React hook form.
But I would suggest moving it to a new task/PR to deprecate it, because we need to try it to see if it works as expected.
|
Closing this since we try another approach. |
No description provided.