accordproject / cicero-word-add-in

A plugin to allow users to interact with Accord Project templates directly in MS Word.
Apache License 2.0
19 stars 9 forks source link

fix: Multiple template handling #44

Closed K-Kumar-01 closed 3 years ago

K-Kumar-01 commented 3 years ago

Signed-off-by: K-Kumar-01 59891164+K-Kumar-01@users.noreply.github.com

Closes #25

Currently, multiple templates with the same identifier can be inserted. We need to ensure that we cannot insert more than one template with the same identifier. The PR aims at that. Now inserting more than one template with the same template won't be possible.

Changes

Flags

Screenshots or Video

Screenshot (50) Screenshot (49)

I tried recording but the debugger was not being shown in the video. Hence, I have shared the screenshots representing the same

Author Checklist

K-Kumar-01 commented 3 years ago

@algomaster99 @irmerk @DianaLease Requesting your reviews.

EDIT: Also, I had an alternative approach where instead of using the XML queries, we can store the template identifers in a state which are already present in the document and iterate over them. IMO, it will be faster as compared to this one though it needs discussion.

K-Kumar-01 commented 3 years ago

P.S. Another good idea would be to integrate pre-commit hooks for the project. :)

@algomaster99 Should I open an issue regarding the same?

algomaster99 commented 3 years ago

Should I open an issue regarding the same?

Yes! Go ahead.

K-Kumar-01 commented 3 years ago

@algomaster99 I have updated my PR. Currently, the toast functionality is not implemented in this PR. I think we can see this as a separate issue as toast need to be used in many places besides here. What do you think about this?

K-Kumar-01 commented 3 years ago

Should I open an issue regarding the same?

Yes! Go ahead.

@algomaster99 I have created an issue for this. Please see if there the description is suitable or not.

K-Kumar-01 commented 3 years ago

@algomaster99 I have updated my code to include the toast in the add-in.

Screenshot (54)

Let me know if any change is required

K-Kumar-01 commented 3 years ago

@algomaster99 I have replaced the toast package. Screenshot (55) Let me know if there are any changes required.

K-Kumar-01 commented 3 years ago

@algomaster99 Any updates on this one?

K-Kumar-01 commented 3 years ago

@algomaster99 I have updated my PR and made the changes. Let me know if there are any more changes required.