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

DM-10523: period finder layout up to date with plotly version #382

Merged
merged 3 commits into from
May 12, 2017

Conversation

ejoliet
Copy link
Contributor

@ejoliet ejoliet commented May 12, 2017

The pull request take care of a version left over from RC and include it in the plotly recently created in dev.
The period finder layout has changed according to the feedback during the test campaign and the version needed to be merged in the plotly version.

Test: bring the LC tool, and go to period finder. The button of the form are now under the control panel, no period value is set by default and title have been added to the plots.

@ejoliet ejoliet requested a review from cwang2016 May 12, 2017 00:40
@tgoldina
Copy link
Contributor

I have pushed a fix for title and selection rectangle not being shown with log scale.

I still see at least one issue: when you select period by clicking on the periodogram, the period in 'Set period' section is populated correctly, but the phase finding chart reflects the previous period value, not the current one. You have to hover over a point for the chart to update to reflecting the current period. (Same behavior with peak table.)

Also, when you first click 'Period Finder', the chart shows with period=NaN day.

Copy link
Contributor

@tgoldina tgoldina left a comment

Choose a reason for hiding this comment

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

Before the merge, please fix the delay in update of phase folding chart. I think it's a simple fix. Just switch the order of lines 391 and 397 in LcPeriodPlotly.js

It would be also nice to fix period=NaN days.

Otherwise looks good.

@ejoliet
Copy link
Contributor Author

ejoliet commented May 12, 2017

Ok, i will try the fix @tgoldina suggest. Thank you.

About the title NaN (days) there are more issue about it and it should be in another ticket. For example, what happen if my data is not in days, etc. See IRSA-354.

@ejoliet ejoliet merged commit f4fa40c into dev May 12, 2017
@ejoliet ejoliet deleted the DM-10523-retrofit-rc-change branch May 12, 2017 20:34
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.

2 participants