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

use refreshIntervalDefaults from config #3977

Closed
wants to merge 5 commits into from

Conversation

Filirom1
Copy link
Contributor

This is an extraction of the refreshIntervalDefaults part of #3908

Right now, this PR failed because of a circular dependency.
Once #3915 merged, this PR will pass.

The field display is needed because it contains the strings that will be displayed in the top bar.
I removed the field section, it's useless.

@elasticsearch-release
Copy link

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

@rashidkpc
Copy link
Contributor

Can you merge master on this how that #3915 has been merged?

@Filirom1
Copy link
Contributor Author

Rebased on master

@rashidkpc
Copy link
Contributor

Merging #3976 broke this one :-) One more master merge and this should be good!

@Filirom1
Copy link
Contributor Author

Rebased once again :-)

@Filirom1
Copy link
Contributor Author

@BigFunger @rashidkpc is it good now ?

@BigFunger
Copy link
Contributor

@Filirom1, I was reviewing this change, and was unable to get it to work the way that I expected it to work. It seems like the config.get call is happening before the stored configuration is being loaded. So, if you make a change in the settings/advanced section, those changes do not get used.

How are you avoiding that issue?

@Filirom1
Copy link
Contributor Author

This last commit will do the job: worldline@6bf7509

This will be the last PR extracted from #3908

But if you prefer, I can include this commit here.

@BigFunger
Copy link
Contributor

@Filirom1, I took a look at worldline@6bf7509

I think it's close, but there's a few small changes that I would like to suggest.

  1. explicitly call the config.init function instead of waiting for the event to fire.
  2. expose that promise as a _.constant init function in timefilter.js

these changes are intended to provent globalState.on('fetch_with_changes'... from being called more than once.

  1. Please follow the existing pattern in the default.js and pre-stringify the json values.

patch gist:

curl https://gist.githubusercontent.com/BigFunger/4d9d86447f36fc6dd174/raw/4394dc947f540abb2da30542cd6810d7ab49766e/patch.txt | git apply

Please review the changes, and let me know if you want me to apply them.

@Filirom1
Copy link
Contributor Author

Wao, thank you.
Of course you can apply this changes.

@BigFunger
Copy link
Contributor

Replaced by #4473

@BigFunger BigFunger closed this Jul 23, 2015
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.

4 participants