Put cloneData function for slate block, in order to change the uuids of fragments #5518
Conversation
✅ Deploy Preview for volto canceled.
|
There was a problem hiding this comment.
@dobri1408 Could you please add a description of the problem you are solving to the PR description? Without it, it is hard to review.
|
Slate lets you put on some of its words(fragments) uuids, that can be used for add-ons, for example for volto-slate-footnotes from EEA. But, if you use the copy and paste of Volto from the sidebar, it will copy the fragments with the same uuids, and this is not ok. @davisagli |
|
@stevepiercy yes indeed |
| ...(data?.value || []).map((c) => { | ||
| return { | ||
| ...c, | ||
| children: (c?.children || []).map((childrenData) => { |
There was a problem hiding this comment.
Won't this add children to nodes that should not have it, like text nodes?
I would really like to see a test for this feature.
There was a problem hiding this comment.
Yes this is true, I will modify, and add tests
| children: (c?.children || []).map((childrenData) => { | ||
| return { | ||
| ...childrenData, | ||
| data: { ...childrenData.data, uid: nanoid(5) }, |
There was a problem hiding this comment.
This will add a uid even if there wasn't one there before; is that what we want?
There was a problem hiding this comment.
I think that all children have uid by default
There was a problem hiding this comment.
No, I don't think so. I have not seen it before.
|
@davisagli what do you think about this way? |
✅ Deploy Preview for plone-components canceled.
|
|
@tiberiuichim maybe you can also give you input on this change. |
|
It's missing a test. This thing would be merged a lot faster if it proved that it did what it claims to do. |
|
Yes, please add tests. |
|
Awesome, were getting somewhere :) If the user knows what he needs todo then he will do it but I would like for us to avoid letting pull requests stay for months until they get so old that it takes a considerable effort to bring up to date. I will try to be more helpful as well to push for my colleagues to ask quicker for feedback so that we can add quickly what is necessary to get a change in a mergeable state and finally merged. Thanks to you both for the feedback, we will see about getting those tests added. |
|
@ichim-david For what it's worth, I asked for tests in December, and @dobri1408 said he would add them, so I thought the next step was clear. I agree that it would be good if we can find a way to respond to pull requests faster, but I'm not sure what to do about the current backlog. Realistically I only have time and energy to do a full review of one or two per day, and not every day. |
@davisagli I thought the test was already written due to the last comment from @dobri1408 #5518 (comment) and due to @avoinea approving the pull request as well. If I can make a small personal request, when asking for further todo's before merging, do it in a dedicated comment, that small comment was in a thread and it seemed like something solved already, but when seeing a test requirement comment you can see if there was any reply given on that individual comment if done or not. |
There was a problem hiding this comment.
@dobri1408 (+ /cc @ichim-david @tiberiuichim)
I'm looking at this again and have some questions:
- What's the goal for how this interacts with the volto-slate-footnote addon? Is the cloned block supposed to refer to the same footnote, or a new copy of the footnote, or no footnote? If no footnote, then I think it would make more sense to remove the uid instead of generate a new one that is not used for anything?
- I'm having a doubt about whether volto should try to anticipate everything that might need to happen to adjust a cloned block. Maybe it should just provide a setting that addons can use to register their own cleanup function.
No description provided.