Skip to content

DM-9589: Fixed WISE case and add basic case with no images #334

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

Merged
merged 2 commits into from
Mar 14, 2017

Conversation

ejoliet
Copy link
Contributor

@ejoliet ejoliet commented Mar 13, 2017

This is a fix for the basic case and neowise tables.

I've introduced a basic case which suppose to handle all numerical column of a general table uploaded in order to allow user to find the period and phase fold, with no images to be displayed.

I have added to the converter cases, 2 new functions, one about the images layout and another one to validate the table uploaded.

The case for WISE is trickier because we need to support tables that contains either frame_id or source_id or scanId and frame_num to be able to display WISE single exposure images. I've changed the WISE plot request and add specific function to validate the table. If the table doesn't have those column, the viewer will bailout in a message (the current message will be reviewed and is not definitive) to let the user know that although the table upload is not valid for WISE/NEOWISE, it can still upload by selecting the basic option.

On table load has changed in order to get the layoutInfo object updated which is acting as changing state to for layout result display, where showImages control the final layout.

To test, please, upload table for different cases and see if the above make sense, otherwise, come to me and let me know your questions.

WISE and Basic case are those that should go for release next week or so.
Generic case has more advanced options and should be reviewed to be included in future releases.

…layed check for basic case, add all numerical column suggestion and band selection radio buttons in WISE case
Copy link
Contributor

@loitly loitly left a comment

Choose a reason for hiding this comment

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

I also noticed that the error message is always appearing in the background, or flashes in some case.
Minor things. Fix if time permits.

</div>
</SplitContent>
<SplitContent>{xyPlot}</SplitContent>
</SplitPane>
Copy link
Contributor

Choose a reason for hiding this comment

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

The table/chart <SplitPane...../> is the same in both cases.. you should reuse it, instead of duplicating it,

@@ -42,7 +42,8 @@ export function makeMissionEntries(tableMeta, layoutInfo={}) {
[LC.META_ERR_CNAME]: get(tableMeta, LC.META_ERR_CNAME, converterData.defaultYErrCname),
[LC.META_TIME_NAMES]: get(tableMeta, LC.META_TIME_NAMES, converterData.timeNames),
[LC.META_FLUX_NAMES]: get(tableMeta, LC.META_FLUX_NAMES, converterData.yNames),
[LC.META_ERR_NAMES]: get(tableMeta, LC.META_ERR_NAMES, converterData.yErrNames)
[LC.META_ERR_NAMES]: get(tableMeta, LC.META_ERR_NAMES, converterData.yErrNames),
[LC.META_FLUX_BAND]: get(tableMeta, LC.META_ERR_NAMES, converterData.bandName)
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look right... FLUX -> ERR. Will you take a look?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good one. Will fix.

const missionListKeys = [LC.META_TIME_NAMES, LC.META_FLUX_NAMES];

const topZ = 9999;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is problematic. 9999 is too high. The validation error message for the same field is showing underneath the suggestion box.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by changing to topZ= 3.

dec: getDEC(layoutInfo),
coordSys: getCoordSys(layoutInfo)};
}

Copy link
Contributor

Choose a reason for hiding this comment

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

the content of getUserInputParams & getUserInputParams look the same?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you mean getUserInputParams and getImagePlotParams are the same, right?

.forEach((pv) => dispatchDeletePlotView({plotId: pv.plotId, holdWcsMatch: true}));

// Decide whether or not to show images, mission specific:
const imagesShown = converterData.shouldImagesBeDisplayed(layoutInfo);
Copy link
Contributor

@cwang2016 cwang2016 Mar 13, 2017

Choose a reason for hiding this comment

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

can this line be moved to the earlier part of this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it needs to be after the plot request object being called.

{revertPeriodTxt}
</button>
</button>*/}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to address ticket DM-9768

@ejoliet
Copy link
Contributor Author

ejoliet commented Mar 14, 2017

I've fixed the popup problem (Loi's catch) and also move the validation of the table to the viewer before the result layout kick in. So it takes an error from LayoutInfo object and check if empty.
Let me know if you find more problems. Thanks!

@loitly
Copy link
Contributor

loitly commented Mar 14, 2017

It only handles error for WISE.
If you upload a bad ipac table, you will get uncaught javascript errors in console and a generic table load error message in where the table would be. Same goes for LSST.
Once this happens, a page reload is needed to recover.

If not fixed here, a ticket(s) should be created to handle all errors.

@ejoliet
Copy link
Contributor Author

ejoliet commented Mar 14, 2017

Error handling was raised here: DM-9770 and is covered for WISE.
For other cases, i think we need to raise a ticket. There is a general ticket here DM-4592 but i guess we need something specific for LC and uploaded table, right?
Will merge this and raise ticket for the other cases.

@ejoliet ejoliet merged commit 2f3a3b2 into dev Mar 14, 2017
@ejoliet ejoliet deleted the DM-9589-remove-constraint-columns branch March 14, 2017 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants