-
Notifications
You must be signed in to change notification settings - Fork 49
Changing the text for wrong file error #256
Conversation
@pinussilvestrus Can you have a look at this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Before trying it out I see three improvements you can do
- Please update your commit message to fit our guidelines. Something like
fix(client/App): update text for wrong file error
Closes #213
- Can you think about a test case which verifies the fix?
- Can you please direct your PR to the master Branch? Bug fixes should always direct to this branch.
bca4eb6
to
17dcdf4
Compare
I made the other two changes but I am not able to think of any test case for this and how do you even make them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggested other, maybe better solutions for the problem. Besides, your commit message is still not valid. The current message is
Closes #213
the suggested message would be
fix(client/App): update text for wrong file error
Closes #213
client/src/app/App.js
Outdated
@@ -2040,7 +2040,7 @@ function getOpenFileErrorDialog(options) { | |||
let seperator = ''; | |||
|
|||
if (isLast) { | |||
seperator = ' or '; | |||
seperator = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You tackled the correct helper function called getOpenFileErrorDialog
. However, the seperator is not the root cause of this problem.
in the Zeebe Modeler we currently only have one single tab provider, that's why providerNames
coming from the options is only including one provider name
function getOpenFileErrorDialog(options) {
const {
name,
providerNames
} = options;
// providerNames = ['BPMN']
// ...
}
Therefore, the logic for getting the providerNamesString
is not working for only one provider name and results in this weird dialog text. So I think a better solution would be either
- Make the helper work in case the array has a size of 1
- Simplify the whole function by only using the single array value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, Can you explain a bit about how to only run the helper function only if the array size is 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how to only run the helper function only if the array size is 1?
why you would want to do it? What I mean is to update the helper function to make it aware for the case the providerNames
is a single value array. Currently, it doesn't do it.
@imrishabh18 any updates on this? Do you need help? |
@@ -2043,6 +2043,8 @@ function getOpenFileErrorDialog(options) { | |||
seperator = ' or '; | |||
} else if (!isFirst) { | |||
seperator = ', '; | |||
} else if(isLast && providerNames.length === 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I'd change the if-else order
if(providerNames.length == 1) {
seperator = '';
} else if (isLast) {
seperator = ' or ';
} else if (!isFirst) {
seperator = ', ';
}
or even outside of the reduce
function
let providerNamesString;
if(providerNames.length == 1) {
providerNamesString = providerNames[0];
} else {
// the current logic
providerNamesString = providerNames.reduce(...)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@imrishabh18 any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I will change the if-else order then @pinussilvestrus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any updates on this @imrishabh18 ? Can we move forward?
I would like to contribute on this card |
I'm closing this since no updates were done for a time now. Feel free to re-open it once we can proceed. |
Which issue does this PR address?
Closes #213
Acceptance Criteria
Definition of Done