Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

Changing the text for wrong file error #256

Closed
wants to merge 2 commits into from

Conversation

imrishabh18
Copy link

Which issue does this PR address?

Closes #213

Acceptance Criteria

  • Corresponds to the concept
  • Corresponds to the design

Definition of Done

@CLAassistant
Copy link

CLAassistant commented Sep 30, 2020

CLA assistant check
All committers have signed the CLA.

@imrishabh18
Copy link
Author

@pinussilvestrus Can you have a look at this?

Copy link
Contributor

@pinussilvestrus pinussilvestrus left a 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.

@imrishabh18 imrishabh18 changed the base branch from develop to master September 30, 2020 16:15
@imrishabh18
Copy link
Author

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.

Copy link
Contributor

@pinussilvestrus pinussilvestrus left a 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

@@ -2040,7 +2040,7 @@ function getOpenFileErrorDialog(options) {
let seperator = '';

if (isLast) {
seperator = ' or ';
seperator = '';
Copy link
Contributor

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

  1. Make the helper work in case the array has a size of 1
  2. Simplify the whole function by only using the single array value

Copy link
Author

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?

Copy link
Contributor

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.

@pinussilvestrus pinussilvestrus self-assigned this Sep 30, 2020
@pinussilvestrus
Copy link
Contributor

@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) {
Copy link
Contributor

@pinussilvestrus pinussilvestrus Oct 19, 2020

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(...)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@imrishabh18 any thoughts?

Copy link
Author

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

Copy link
Contributor

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?

@vvkpd
Copy link

vvkpd commented Nov 1, 2020

I would like to contribute on this card

@pinussilvestrus
Copy link
Contributor

I'm closing this since no updates were done for a time now. Feel free to re-open it once we can proceed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong Error Message Text When Attempting to Open DMN
4 participants