Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolved issue #173 - preventing AIA upload as media asset #747

Closed
wants to merge 19 commits into from
Closed

Resolved issue #173 - preventing AIA upload as media asset #747

wants to merge 19 commits into from

Conversation

varunm22
Copy link
Contributor

Resolved issue detailed at #173 and replaced window.alerts with gwt dialog boxes.

@afmckinney @jisqyv

}
});
holder.add(info);
} else if (errorType.equals("noFile")) {
Copy link
Member

Choose a reason for hiding this comment

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

You should get rid of this bunch of if ... then else if ... stuff. Instead just pass the error message to the dialog. as in:

createErrorDialog(MESSAGES.noFileSelected(), folderNode, fileUploadedCallback);

-Jeff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @jisqyv,
I mainly sent the error type so I could tell whether or not a specific error type needed other buttons. For example, the aia upload error requires a "More Info" button. Should I just have something that checks if the sent error message is equal to MESSAGES.aiaMediaAsset() and adds the More Info button if so?

Copy link
Member

Choose a reason for hiding this comment

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

@varunm22 OK, I see what you are attempting to do. However it would be much cleaner to provide the error message as an argument, as I have suggested, and then add an additional errorType argument which should be a Java enum type. Then instead of a bunch of if .. then .. else .. if statements, you can use a switch statement based on the enum to determine what additional buttons should be placed, etc.

@jisqyv
Copy link
Member

jisqyv commented Apr 12, 2016

@varunm22 See line note above.

@varunm22
Copy link
Contributor Author

Hi @jisqyv, I fixed up the code with your suggestions, please let me know if there's anything else I can improve!

@jisqyv
Copy link
Member

jisqyv commented Apr 30, 2016

@varunm22 Looks good... except... If you look at other components, you will see we use ALL UPPER CASE for enum values. Can you make that change?

@varunm22
Copy link
Contributor Author

varunm22 commented May 1, 2016

@jisqyv I fixed it, thanks!

@jisqyv
Copy link
Member

jisqyv commented May 1, 2016

Thanks. I'll do my final testing this evening and if all goes well I'll
merge it into the master branch.

-Jeff

On Sat, Apr 30, 2016, 11:55 PM Varun Mangalick notifications@github.com
wrote:

@jisqyv https://github.com/jisqyv I fixed it, thanks!


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#747 (comment)

@jisqyv
Copy link
Member

jisqyv commented May 2, 2016

@varunm22 LGTM

There are some whitespace errors, but I'll take care of these when I squash things down for Gerrit

@jisqyv
Copy link
Member

jisqyv commented May 2, 2016

@varunm22 MERGED

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

Successfully merging this pull request may close these issues.

None yet

2 participants