Isaac, I really appreciate for all your efforts . Your patch looks good just that the refactoring of the code seemed to be a point of concern . I tested your patch and it seemed to be working fine . Perhaps you could send an updated patch by removing the whitespace errors and I can push it to asf/master.
Thanks, Pranav -----Original Message----- From: Isaac Chiang [mailto:nore...@reviews.apache.org] On Behalf Of Isaac Chiang Sent: Thursday, January 17, 2013 7:05 PM To: Pranav Saxena Cc: cloudstack; Isaac Chiang Subject: Re: Review Request: CloudStack-965: When a detailview action is prohibited, the operation dialog box should not show up in the mean time > On Jan. 17, 2013, 10:15 a.m., Pranav Saxena wrote: > > Your code changes look good . However , I have few concerns - > > > > 1) The patch doesn't apply cleanly because of trailing whitespace errors . > > Hence could you take care of these - > > > > home/pranav/Downloads/fix-cloudstack-965.patch:25: trailing whitespace. > > // Setup form validation > > /home/pranav/Downloads/fix-cloudstack-965.patch:27: trailing whitespace. > > $formContainer.find('input, select').each(function() { > > > > /home/pranav/Downloads/fix-cloudstack-965.patch:31: space before tab in > > indent. > > else { > > /home/pranav/Downloads/fix-cloudstack-965.patch:32: space before tab in > > indent. > > $(this).rules('add', {}); > > /home/pranav/Downloads/fix-cloudstack-965.patch:33: space before tab in > > indent. > > } > > > > 2) Is there any specific reason for moving/reorganizing a section of code > > in dialog.js from one point to other , though I know it won't affect > > anything. I believe , you could implement the same without moving the > > chunks of code here and there , just to maintain the code consistency and > > ordering. Please let me know if you have any concerns. > > > > Thanks ! > > Hi Pranav: The root cause of the issue, as I saw, is when the font-end is trying to make a dialog form, it takes fields from from the caller ( detailview->createForm->fields ). Once the field block contains an ajax call, e.g., migrate in instance.js : 867, It emits it. But it still do the rest of the code in createForm and shows the dialog form. I figured out few ways to solve the issue, including refactor entire workflow for creating the dialog form, but those are more than I can do right now. The patch isn't good but merely a tradeoff in my situation, just let you know. No matter what, hope the piece of code will help to identify the issue. I'll close the review request if it achieves its purpose, thanks :) - Isaac ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8992/#review15452 ----------------------------------------------------------- On Jan. 17, 2013, 8:09 a.m., Isaac Chiang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8992/ > ----------------------------------------------------------- > > (Updated Jan. 17, 2013, 8:09 a.m.) > > > Review request for cloudstack and Pranav Saxena. > > > Description > ------- > > The patch is to resolve CLOUDSTACK-965 issue which prevents from showing the > error message and dialog form in the mean time. > > > This addresses bug CLOUDSTACK-965. > > > Diffs > ----- > > ui/scripts/ui/dialog.js 5236bb6 > > Diff: https://reviews.apache.org/r/8992/diff/ > > > Testing > ------- > > > Thanks, > > Isaac Chiang > >
