-
Notifications
You must be signed in to change notification settings - Fork 16
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
Conversation
…layed check for basic case, add all numerical column suggestion and band selection radio buttons in WISE case
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 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> |
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.
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) |
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.
This doesn't look right... FLUX -> ERR. Will you take a look?
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.
good one. Will fix.
const missionListKeys = [LC.META_TIME_NAMES, LC.META_FLUX_NAMES]; | ||
|
||
const topZ = 9999; |
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.
This is problematic. 9999 is too high. The validation error message for the same field is showing underneath the suggestion box.
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.
Fixed by changing to topZ= 3.
dec: getDEC(layoutInfo), | ||
coordSys: getCoordSys(layoutInfo)}; | ||
} | ||
|
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.
the content of getUserInputParams & getUserInputParams look the same?
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 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); |
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.
can this line be moved to the earlier part of this function?
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.
no, it needs to be after the plot request object being called.
{revertPeriodTxt} | ||
</button> | ||
</button>*/} |
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.
This is to address ticket DM-9768
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 |
It only handles error for WISE. If not fixed here, a ticket(s) should be created to handle all errors. |
Error handling was raised here: DM-9770 and is covered for WISE. |
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.