OBPIH-5120 Show that loading is in progress on substitution window #3723
Conversation
| @@ -3036,6 +3036,7 @@ react.stockMovement.edit.label=Edit | |||
| react.stockMovement.pick.label=Pick | |||
| react.stockMovement.pack.label=Pack | |||
| react.stockMovement.send.label=Send | |||
| react.stockMovement.noSubstitutionsAvailable.message=There are no substitutions available | |||
There was a problem hiding this comment.
I think it won't work if it doesn't have .label at the end
There was a problem hiding this comment.
why? Do you mean crowding will not detect this label ?
I am pretty sure there are many labels that end with .message that we have in the project and it looks like all of them have translations
There was a problem hiding this comment.
I remember not a long time ago I didn't add .label and the translation did not work, but I do not rememeber if it was on gsp pages or react
There was a problem hiding this comment.
but it could be a random case that it didn't want to work for me, so if you are confident that it should work, I'm fine with it
There was a problem hiding this comment.
I think we should leave it as it is and if after merge it turns out that it doesn't work then we will know for sure :D
There was a problem hiding this comment.
I checked locally this branch and the translation seems to be working fine even if removing .message etc, so my case when it did not work without .label was probably a random one. I apologize for confusion
There was a problem hiding this comment.
what I meant at the beginning is that the label won't even be recognized, so that it'd take the defaultMessage, not that it will work, but will not be translatable with crowdin, but never mind, everything is fine
| const { | ||
| emptySubstitutions, | ||
| loadingSubstitutions, | ||
| } = properties; |
There was a problem hiding this comment.
I think you should add a comment about usage of those properties and also about the two ifs below, as TableBody is a component we use in many places and this particular case will only be used for substitution modal
There was a problem hiding this comment.
I also think that such a request might come in some other places/modals, so maybe we could think about some "generic" naming here? @awalkowiak what do you think?
There was a problem hiding this comment.
@kchelstowski @alannadolny yes, this definitely has to be generic
| return (<Spinner />); | ||
| } | ||
|
|
||
| if (emptySubstitutions && !fields.length) { |
There was a problem hiding this comment.
what is that fields.length responsible for here? fields is probably an object, so I do not understand the usage of that? Isn't checking emptySubstitutions good enough?
There was a problem hiding this comment.
Yes, fields is an object, but it has its length as a property. Checking emptySubstitutions isn't enough, because when you are clicking 'add custom substitution' emptySubsitutions is still 0, but fields.length will be > 0, so message with 'There are no substitutions available' will disappear. When it will be only emptySubstitutions there will be displayed this message and added substitution.
There was a problem hiding this comment.
oh, I see, I forgot about adding a custom one, then I'm fine with it - it was rather a question, not a request change
| } = props; | ||
| const dynamicAttr = getDynamicAttr ? getDynamicAttr(props) : {}; | ||
| const attr = { ...attributes, ...dynamicAttr }; | ||
|
|
||
| this.state = { | ||
| loadingSubstitutions: false, |
There was a problem hiding this comment.
I can't see emptySubstitutions being initialized here?
| formValues: { | ||
| substitutions, | ||
| }, | ||
| originalItem, | ||
| }); | ||
| }).catch(err => err); | ||
| }) | ||
| .catch(err => err); |
There was a problem hiding this comment.
don't we want to mark loadingSubstitutions as true if we get an error also? In my opinion if any error occurs, the spinner will still be visible.
You could use .finally in the end, so you don't have to hide the spinner twice (you would avoid setting the loading to false twice - once in .then and once in .catch)
|
Yes, I know that code formatting can be problematic and I am trying to avoid it, but this is my habit, so I forget about it sometimes. I will pay more attention to code formatting. |
| // fields.length is checked due to possibility of adding custom substitution | ||
| if (emptySubstitutions && !fields.length) { | ||
| return (translate( | ||
| 'react.stockMovement.noSubstitutionsAvailable.message', |
There was a problem hiding this comment.
This should be passed from the modal component (or other component using the TableBody)
No description provided.