-
Notifications
You must be signed in to change notification settings - Fork 439
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
Controlled component behavior in React wrapper #489
Comments
This "controlled" behavior could also greatly enhance the experience with other view library/framework wrappers |
The only super confusing part would be how to handle animations during state changes, especially since tagify would no longer have any idea of identity (especially if you have duplicates enabled and have duplicate tags). The animations make everything a lot harder. I have seen other libraries that let you manually set the complete contents like the "setTokens" method this: http://sliptree.github.io/bootstrap-tokenfield/#methods which I saw here #45. It doesn't look like they have animations though. |
Also, rendering things onto the DOM between state changes would require either: a manual deletion and recreation of all tag DOM elements on every state change, intelligently determining how/if to mutate the DOM by yourself, or intelligently determining how/if to mutate the DOM with something like React. At that point having this library operate in a "controlled" manner would likely cause you to need the assistance of a view library which kinda makes me think that the only elegant way to achieve this would be to build the library from the ground up in React. These are just my thoughts. I have written react wrappers myself but this scenario is super awkward. I would be willing to help out if needed. |
You could have it so that state is formed as such: <Tagify That way identity can be maintained for animation purposes, especially when duplicates are enabled. This guid information would be provided along with the onAdd, onChange, onDelete events. Also obviously the onAdd, onChange, and onDelete handlers would fire only when something was modified directly inside the tag input/list and not for additions/modifications/deletions that were made as a result of react passed in state changes. |
Like where it can operate like this for example
|
Currently when you update the it's not intelligent at all, but in terms of performance totally negligible because I cannot imagine thousands of tags exists and re-created. I think that below a couple hundred tags it's totally fine. I'm trying to keep the core code from being bloated, and am very hesitant on adding new features/requests which requires me to write a lot of new code. If you want to disable the tags removal transition, you can set the CSS variable |
I agree that the "load original values" thing isn't an issue and makes sense. The only issue is that feeding state back into the component literally doesn't work. No react library or wrapper I have ever used in my years of react development has the sort of deficiency I describe below. Consider this example:
The desirable and proper react behavior for this would be that regardless of what tag you attempt to create/delete/change, the form would not make the change but instead immediately set the tag input to show a tag that says "Some value!!!". |
ok ok no need to get angry, I understand you want a "controlled" component, let me think how can this be done. But can you at least explain to me why do you want it to behave like that? |
My bad!! I didn't mean to come off as rude. Having the onChanged event set the react state which feeds back into 'value' prop causes a double firing of events (namely when you press backspace to delete a tag). You also get a firing of the onchange event when initializing if you provide a 'value' prop. I also get a react warning saying "can not flush updates while react is already rendering" every time a tag changes. In general, the reason controlled behavior is so important is we have a very complex react + redux application and the tag input needs to be properly synced with single source of truth. Updating the tags needs to be done bidirectionally (sometimes through input itself, some times changed by redux). It makes it really hard to do this if library is not well suited towards "controlled" behavior. On a side note: It would also be a very nice convenience if the callback would provide the tag list as an array as opposed to having the user use event.target.value to get a string of the array which then needs to be converted into an array using something like JSON.stringify. Giving it directly as an array would lend itself very well to being a controlled component. As for approach, I'm a fan of how this library implements controlled behavior: https://github.com/prakhar1989/react-tags |
Tagify was built in javascript and not in React and in vanilla-world, there are no controlled-components, so the wrapper doesn't magically transforms Tagify to be controlled, it a complex thing I must work on internally. That other lib you linked to, which you've mentioned you're a fan of doesn't have close to half the features Tagify has. I will make it work, just need to think what would be the best way to tell the component it shouldn't add/remove/edit tags by itself, but only fire a "change" event with the new-to-be value and then it is up to you as the developer to update your state and re-feed the component with the new value. This won't be as performant as letting tagify handle the DOM because it knows best only to update what's needed and not everything entirely every time there's a change, but if this is what you absolutely need.. In my opinion you don't need to be so strict about this and just give up and let Tagify do what it does and you just make sure you keep your state in sync and not the other-way around. What does it matter anyway... it's not like it could get out-of-sync. You can always change the |
That would be awesome |
Thanks kind sir |
I am familiar with difference between a library that was built in react vs. a library that is built in vanilla that uses a wrapper to interact with the underlying javascript object in response to changes in props. I think similar to the library I mentioned (which admittedly does have a lot fewer features) would be to have
The issue I see with just providing the all values via the onChanged method versus providing separate onAdd/onRemove/onChanged is that the person that is going to mutate the state themselves would likely want to know exactly how the input should change vs. just knowing what it would change to if tagify instance were to do its stock behavior. My particular use case would greatly benefit from onAdd/onRemove versus just using onChange. I understand that the 'controlled' behavior would potentially big changes to your core tagify library and the stuff I am saying is a lot easier said than done. The best way I see this being done is having an "option" for controlled mode that will get passed in as an extra when we initialize tagify from our react wrapper (this thing: new Tagify(inputRef.current, { callbacks, ...options } || {})). This will inform the core library that the instance of tagify that we just created will be self managed. As you mentioned, loadOriginalValues is not bad at all! The way native React libraries mutate the DOM vs. the way loadOriginalValues just recreates all of the DOM elements is completely unnoticable as far as performance goes in this scenario of not :) I fully understand that the only way to get this mutating behavior would be to rewrite the library in pure react which is completely impractical and for virtually no benefit. |
As far as a use case for controlled behavior vs uncontrolled behavior in the context of using this library:
Pardon my bad code. I want to display a toast when the user attempts to add a tag that goes beyond the limit or has too many characters. To achieve this, I leave the the maxTags property blank. With controlled behavior when a tag is attempted to get added I would be able to reject it (don't add to single source of truth) and show a toast. Ideally since I'm using redux the onAdd event would trigger an action which would then get received and processed by redux-saga. The saga would determine if the new entry that was attempted to get added wouldn't cause me to exceed the maximum number of tags and if it doesn't exceed the maximum number of characters allowed inside a tag. If too many tags I want to dispatch an action from redux to display a toast with a "too many tags" text, if tag is too long in character length I want to dispatch a toast with a "tags must be x length" message, or if tag successfully fits all criteria I want then to dispatch action to redux to add to the tags state array. Giving me just a list of all tags through the onChange event limits me into being able to distinguish where the tag was added. I understand that you have a maxCharacters and maxTag attribute but then using these puts you at the mercy of your core library behavior. Even worse, if you wanted to implement something even more custom (like say you only want to allow tags to contain certain characters), then you would be completely out of luck whereas having the information to make these decisions yourself and a controlled component would allow infinitely customizable behavior for react folks that need it with no need to add on to your core library. Note that the wrapper could also benefit from being able to detect changes to the placeholder and apply them accordingly. This would allow me to pass Attached is the behavior that I try to achieve with the above code. Debatably toasts are not the best UX for this scenario but you see how this could apply to other people's use cases as well. It does work but could be much more streamlined if there was controlled behavior: https://streamable.com/gqqoyv |
Tagify now supports hooks, not React hooks, but "hooks" as the term was originally used before React or any other framework existed. Developers used to name certain placed in their code where other developers could intervene and chose how to proceed and in what manner. I've only made a "beforeRemoveTag" hook, but the demo clearly shows how it can be used to stop Tagify from removing a tag. I actually did code an "beforeAddTag" hook but have decided to delete it from the code because I couldn't think of a use-case scenario. You should not forget tags may be editable, so you also need an "onBeforeEditDone" hook, so this means now you will need to listen to 3 hooks (add, remove, edit) to change manually sync the state and re-feed Tagify with new value (to force it to be "controlled"). Also, you must take into consideration that Taigy has other modes: "mix" & "select" for which you should also address when making a "controlled" component, because I do not want to release a version which only supports "regular" tags as "controlled". Therefore I have concluded that developers should have a simple "channel" of communication with Tagify through
Now, if you want a "controlled" component, then I think the best option is to pass this desire in Tagify's "settings", as a boolean, and this new setting will internally stop any DOM modification done by Tagify, but will fire a This is all in in theory, I'm not even sure it's fully feasible but I can try, when I get to it... |
Tagify fires an Invalid tags are only rendered but not added to the
As a UX/Product person, I do not like the idea of a user adding a tag and then wait an arbitrary amount of time before seeing it added. A server request might be slow and the user will be left in the dark, or worse, try to add the same tag again.
Tagify doesn't have Please I want to know a scenario which tagify cannot handle, please let me know of any. |
Tying into your hooks is a pretty solid approach. If you had beforeAdd, beforeEdit, and beforeDelete hooks you could have it so the react wrapper exposes these hooks as -> onAdd, onEdit, and onDelete. The wrapper would trigger the passed in onAdd, onEdit, or onDelete callback the user provided while providing their callback function with all appropriate information like proposed value, index, etc etc, and always immediately provide the beforeX hook with a return value of false value so it doesn't actually add it to the dom elements directly from the underlying tagify instance. If this performs well in real life usage and there are no weird visual glitches it would achieve a very react-like controlled experience with minimal modification required to the code. Also here is the flush updates warning I was talking about. It happens on your crazy tags demo: In general I hear you loud and clear about your library having lots of built in functionality like invalid event, etc. I just think in general it would be cool to have full control over these tags and treat it as a 'dumb' component instead of using all the built in functionality though, especially since it lets you decouple some of your logic from this particular tag library and lets you test aforementioned custom logic very easily. |
I've also seen these |
FWIW I was seeing those weird errors locally, but it looks like they are gone after upgrading from |
I'm using a standard controlled component (I think ;) ) implementation where the parent component passes values as props, and the child component uses <Tags, passing the props value and onChange. onChange is a callback to the parent, which changes its state.value and passes state.value as props to the child component. This leads to infinite callbacks. Is this related to this issue? I don't have as much React / front-end expertise as you guys so I haven't fully followed this discussion. |
@Spoonrad The problem that I see is that every time you change the value, a new value is passed to I'm not exactly sure what would be the best way to handle, but one option would be to compare current value with what's received If it's the same, don't change the state. Also, the "value" provided seems to be the same for every call (e.g. it mutates the value contents without creating a new one) so you have to do some depth comparison (not a shallow one). This compare logic would be better the |
I use this implementation with my antd components, which are derived off react-components, so I was just a little bit confused about why this was happening. It's not a very big deal, I get the initial values as props in Child, pass them to the Child's state in its constructor and the Parent's state gets updated when a change happens. It still passes the value as props but that is only used once, in the Child's constructor. So it works, it's just not super recommended by React AFAIK. |
I can internally do a deep-compare in the React wrapper and only proceed if new state is different
|
@yairEO I haven't found any particular benefit to using hooks, except the minor benefit that it avoids component injection but that's about it. I don't really see the point of learning & migrating a new syntax if it doesn't have any benefit, and there is no planned deprecation of standard components Just looked at some articles and I can see why hooks are useful, but their usage doesn't really respond to a need currently. We'll look into it, thanks for the feedback. :) |
One benefit is others will be easily able to read your code. I used to work with React classes before hooks and now I don't remember much at all, after not seeing class components for over 3 years. React Hooks are so much more elegant IMHO, I love them so so much. Anyway, I've pushed a fix to the Tagify React wrapper so new values will be compared to current ones and only if different, should render the new ones |
I can definitely think of scenarios where you want the component to be controlled by the way. I'm trying to set a form up that contains an input with Tags and it's been a nightmare. I'm still stuck with infinite loops, even if the I thought that making sure to always pass the same I've updated to 3.25 and no change. The sandbox example posted above doesn't work either with 3.25 |
@Spoonrad - what are the steps I need to do in order to reproduce the issue? Currently the onChange = (value) => {
// Uncommenting this
// makes everything break
// with infinite callbacks
this.setState({
value: value,
xxx: 'c'
});
}; I'm adding/removing tags and don't experience any infinite loop
I would be happy to hear. Tagify handles its own DOM, it will add/remove/edit nodes by itself and all a developer needs to do is listen to the |
Typically what I'm used to doing in a form is the implementation in the sandbox (sorry I just edited it so the loop can happen again). A parent maintains an object state and has form inputs as its children. It passes the state as props to the children, which then pass the changed value back to the parent, which changes its state. I'm used to doing things this way and right now I'm struggling a little with Tagify because of the issues with onChange. I'm probably going to stop trying to use onChange and instead get the values by using a ref, but that's usually not a recommended implementation. |
Same here. I did this because I wanted to use my own business logic. It seems like this library doesn't want you to use it in a controlled fashion because it really wants you to rely on its own business logic implementations |
@agusterodin - This is a vanilla library that has a simple React wrapper to easily integrate Tagify within JSX. I have used Tagify myself, in very large scale, highly complex, React systems, without any issue. @Spoonrad - it is not Tagify's fault here but your own code. You are setting the state in a way which is forbidden in React - mutating it and not creating a completely new Object on every state change, this is why Tagify does not register the changes of the props: onChange = (value) => {
this.setState({
value: value // never do this
});
}; You are simply pointing the state back to Tagify's internal state. React cannot compare the previous state with the new one in this way, since the previous once also was point to the *exact same Object! This is a crucial fundamental React thing right here. Another problem with your code is that you did not use my latest version at all, but used some local file instead. See your import declaration for the import Tags from "./tagify/react.tagify"; // you use this
import Tags from "@yaireo/tagify/dist/react.tagify"; // when you need to use this There are many other issues with your code, I had it simplified and here, everything works, and the component is in a controlled manner. Here's the demo: |
Thanks a lot for the feedback and help!
Just please note that I started off your CrazyTags sandbox and didn't pay attention to the import statement, I just changed the dependency version Your demo works great! You have been super helpful and I am very grateful for your help. Could you please explain to me what is wrong with the slightly modified code in this demo? The transformation here isn't useful but basically the object in the Parent state is different from what is passed to Child (in my real-life example), so I'm trying to figure out why it doesn't work in the case above. Thanks again for the help / feedback. BTW your demo works fine without the shallow cloning, what fixed it was the version change. With older version: With newer version: |
Yes, I have. |
@agusterodin - then you must know that it's impossible to change already-exiting code of the vanilla script from the wrapper itself, therefore the wrapper can only play by the rules of the script it wraps, which means the wrapper is not in charge of the render process, which finally means there's no real way to make the component truly "Controlled". All the wrapper can do is send commands to Tagify, one-way communication. |
@yairEO Sorry for the second ping but since you reacted with 🎉 to my message I wanted to make sure 😁 I'm still stuck with this "real-life" example: Basically I need to do some transformations from Parent to Child and from Child to Parent, so I'm trying to pass lists of objects that have the same data structure as Tags; so Parent sends [{value, ...}] and Child sends back [{value, ...}] (in this example the structure is the same). What seems to make things break is when Child passes [{value, ...}] instead of tagsInstance.value, even though you would expect it to work. |
|
@Spoonrad - I have worked for the past hours to help you in your case and improved some internal mechanism in Tagify and have published a new version Here's the demo: |
@yairEO Thanks for all your quick work on this, definitely appreciate it! I'm going to look into this new version very soon and let you know if I was able to solve my problem. Does this mean that now it is best not to use the tagifyInstance.value but rather parse the value passed through event.detail? |
I have introduced a new method to tagify, called The |
OK, it works well with With JSON.parse() it doesn't work (the tags are reset when you leave the input box) but with getCleanValue() it does. I have the following warning though:
|
I will fix this warning. What do you mean by:
Does the color comes from some back-end? If not, then it's better to add the color through the Tagify "proper" way, using the |
Yes it comes from a back-end, that’s why it’s complicated to use transformTag (which I tried). Thanks for the feedback, everything seems to work now!
|
I'm getting the warning as mentioned by @Spoonrad 2msgs above. Installed the dependency with NPM today. Component code below: import React, { useCallback } from 'react'
import Tags from "@yaireo/tagify/dist/react.tagify"
import "@yaireo/tagify/dist/tagify.css"
import styles from '../../styles/TagifyInput.module.css'
export default function TagifyInput({ tags, setTags }) {
const onAdd = (e) => {
console.log(e)
}
return(
<Tags
settings={{delimiters: ";"}}
placeholder='Insert team tags'
className={styles.customLook}
onAdd={onAdd}
/>
)
} I'm also having another problem, I'm having to click on the input field of Tags exactly 3 times before I could maintain focus :( |
I don’t think the warning is there in my project anymore, I don’t believe I have seen it again but I may be wrong. I’m making a quick implementation because I need something testable quickly at the moment. |
Hi @sweetsoul, if you think Tagify has a bug and you can reproduce the bug in a demo page, then please open a new issue |
The problemA bit late to the party, but I'm facing an unusual issue as well when trying to emulate controlled behavior with the React wrapper. I maintain two parallel mix-mode Now, I am able to share the state successfully between the components, but, I noticed that whenever I add a new tag - the caret position gets sent to the beginning. If I remove the You can test out this behaviour in this CodeSandbox Solution?After checking out the useEffect(() => {
const currentValue = tagify.current.getInputValue()
if (mountedRef.current && !isSameDeep(value, currentValue)) {
tagify.current.loadOriginalValues(value)
}
}, [value]) Every time the @yairEO thoughts? |
@agrawal-rohit - you can just not update the |
@yairEO, that's interesting. Would it be possible for you to update this CodeSandbox with a possible solution? It'd be a huge help 😇 |
@agrawal-rohit - sorry but I'm swamped with work at the moment and it will take me days to get to it.. |
That's fine, It's not super urgent 😇 |
@yairEO could you add a solution in the sandbox if you get some time? |
@yairEO - really liking Tagify so far! I have very little React experience, so apologies in advance if the question below is a bit silly. I've put my question here, because I'm not sure if it's related to controlled component behaviour. I'm currently using the React version, by copying the MixedTags example here. It seems like OnChange only triggers when a tag is selected. How would I go about making OnChange trigger for something like OnKeyPress as well? The reason I'm looking to do this is so that I can read in the current value at all times (for a Plotly Dash app callback), not just after selecting a tag. Currently logging looks like the below: This poses an issue if I were to stop at "@tag1 is small", because the last changed value would still look like the first logging above. |
Instead of relying on callbacks to gather information about what happened as far as tag additions/changes/deletion, the tag input should be driven off of React state (ie: an array of tags as a single source of truth). Right now the source of truth is self contained within the tagify instance itself. Right now it behaves as an uncontrolled component whereas a controlled component would be ideal!
To make this possible, you would need to add a function that lets you manually set the contents of the tag input array (pass an array and it will change the contents of the tag input to reflect this new array).
It would also involve adding an option to instance factory that tells the library not to automatically add/remove/change the tag in its own internal tag state, but still provide information on what were to happen in handler functions so that we are given the necessary information to know how to alter the react state (which in turn will cause the controlled tagify component to be updated based on the array from state we feed into it).
The text was updated successfully, but these errors were encountered: