#701 word forms change in collect element issue #710
Conversation
Updated as requested in issue #701
Now it will properly update the last added verb in the collect bar
|
🎉 Beta deployment successful!: view the changes in live preview environment: https://gridbeta-github.asterics-foundation.org/alexangelov14/-701-word-forms-change-in-collect-element-issue/pr |
klues
left a comment
klues
left a comment
There was a problem hiding this comment.
Thanks! 👍
Please see my comments.
| case GridActionWordForm.WORDFORM_MODE_CHANGE_BAR: | ||
| collectElementService.addWordFormTagsToLast(action.tags); | ||
| stateService.resetWordFormIds(gridElement); | ||
| stateService.addWordFormTags(action.tags, action.toggle); |
There was a problem hiding this comment.
I don't understand why we need stateService.resetWordFormIds and stateService.addWordFormTags here? Honestly I cannot really remember exactly what they are doing, but from my current logic, should'nt it be enough to have the adaption in collectElementService.addWordFormTagsToLast() below?
There was a problem hiding this comment.
Technically its enough but with the change in my opinion its better, because what it changes is the following (will show an example):
The user chooses first "I" then wants to change "be" to "am". How it was before you should've pressed first "be" for it to be added to the collection then press "1.PERS" to make it to "am". Also the element itself always stays as "be". If the user presses the "1.PERS" element while nothing is in the collection bar they might start wondering what that even does as its not reflected anywhere else. Personally i don't think that is a great user experience (of course thats up for a discussion). With the current change it goes like this. The user chooses first "I" then wants to change "be" to "am". Now he will first press the "1.PERS" then he will see how the element itself changes to "am" and can decide if thats the right choice before pressing it. After the user presses the now changed "be" to "am" element it will be added to the collection bar as "am" and it will revert itself back to the normal "be" on the element. Hope that clears it out.
There was a problem hiding this comment.
thanks for the explanation.
There are different modes for chaning the word forms in the "change word forms action":
So there are defined modes for:
- changing the word forms in the elements
- changing them in the bar
- or changing them everywhere
Also see the demo for word forms here: https://grid.asterics.eu/?gridset_filename=grammar-demos.grd.json
So I think with this change your changing functionality of the mode change in bar to the (already existing mode) change everywhere - which shouldn't be the case - because you can use the existing other mode if you want this behaviour.
| collectElementService.addWordFormTagsToLast = function (tags, toggle, skipLast) { | ||
| // If skipLast is true, update the second-to-last element instead of the last one | ||
| // This is useful when the triggering element was just added to the collect bar | ||
| let targetIndex = skipLast ? collectedElements.length - 2 : collectedElements.length - 1; |
There was a problem hiding this comment.
I wonder if we should use a different approach here. maybe add some param like skipElementId and then pass gridElement.id when calling the method. Then use the last element of the collected elements which has not the ID of skipElementId. The result will be the same, but the approach is more robust, since it doesn't depend on the order of execution. This solution now only works, if the new element was already added at this point (which seems to be the case right now), but not if it would be added after executing this function. Since it's just chance that the code is executed in this order right now and it's not guaranteed to stay the same forever, the other approach with skipElementId would be more robust and would still work, if code execution order changes for any reason in the future.
|
🎉 Beta deployment successful!: view the changes in live preview environment: https://gridbeta-github.asterics-foundation.org/alexangelov14/-701-word-forms-change-in-collect-element-issue/pr |
|
@klues i added the changes requested in the second comment. You can check it out. |
|
@klues hello could you check the last changes whenever possible? |
| collectElementService.addWordFormTagsToLast(action.tags); | ||
| stateService.resetWordFormIds(gridElement); | ||
| stateService.addWordFormTags(action.tags, action.toggle); | ||
| collectElementService.addWordFormTagsToLast(action.tags, action.toggle, gridElement.dontCollect ? null : gridElement.id); |
There was a problem hiding this comment.
What speaks against just always passing gridElement.id here, also if it has dontCollect set. Then this id will be skipped, but is not in the collected elements, therefore again the last element will be used?!
| // This is more robust than skipLast because it doesn't depend on execution order | ||
| let targetIndex = -1; | ||
| for (let i = collectedElements.length - 1; i >= 0; i--) { | ||
| if (!skipElementId || collectedElements[i].id !== skipElementId) { |
There was a problem hiding this comment.
somehow this seems wrong to me. If I don't pass skipElementId then finally the targetIndex will be 0, so the first element. But I want to change the last element in this case?!
If I'm right, I think if skipElementId is not pased the targetIndex should be collectedElements.length - 1, shouldn't it?
ah, ok, now i understand - there is the break below. I think it would be much easier to understand if you would have if(!targetIndex && collectedElements[i].id !== skipElementId) here (and setting targetIndex = null above). Then no break is required and it's clear that I only set the targetIndex, if it's not already set (= taking the first one matching the criteria).
| } | ||
| } | ||
|
|
||
| if (targetIndex < 0) { |
There was a problem hiding this comment.
then we would need !targetIndex here
These are changes for issue #701