Skip to content
This repository has been archived by the owner on Aug 9, 2022. It is now read-only.

Use commonlyUsedRanges from Kibana Settings instead of a constant value #352

Closed

Conversation

iget-master
Copy link

Issue #, if available: #351

Description of changes:

  • Add the uiSettings service from Data plugin as dependency
  • Removes unused dependencies (notifications and navigation)
  • Change time_range component to use commonlyUsedRanges from uiSettings (following default behavior for EuiSuperDatePicker across entire kibana), allowing user to override defaults
  • Removed the constant that was used for commonlyUsedRanges

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@iget-master iget-master changed the title Issue 351 Use commonlyUsedRanges from Kibana Settings instead of a constant value Apr 5, 2021
@codecov
Copy link

codecov bot commented Apr 5, 2021

Codecov Report

Merging #352 (4c9a549) into dev (98750be) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #352      +/-   ##
============================================
+ Coverage     64.12%   64.16%   +0.03%     
  Complexity      291      291              
============================================
  Files           100      101       +1     
  Lines          4081     4085       +4     
  Branches        622      621       -1     
============================================
+ Hits           2617     2621       +4     
  Misses         1304     1304              
  Partials        160      160              
Flag Coverage Δ Complexity Δ
Kibana-reports 77.35% <100.00%> (+0.04%) 0.00 <0.00> (ø)
reports-scheduler 53.28% <ø> (ø) 0.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...rt_definitions/report_settings/report_settings.tsx 89.32% <ø> (ø) 0.00 <0.00> (ø)
...ions/report_settings/report_settings_constants.tsx 100.00% <ø> (ø) 0.00 <0.00> (ø)
.../report_definitions/report_settings/time_range.tsx 53.19% <100.00%> (+1.54%) 0.00 <0.00> (ø)
kibana-reports/test/uiSettingsMock.js 100.00% <100.00%> (ø) 0.00 <0.00> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98750be...4c9a549. Read the comment docs.

http: CoreStart['http'];
navigation: NavigationPublicPluginStart;
// navigation: NavigationPublicPluginStart;
Copy link
Member

Choose a reason for hiding this comment

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

If navigation and notifications are not used anywhere, please remove them from code, instead of comment it out.
@joshuali925 @davidcui-amzn could you confirm that it's safe to remove those 2?

Copy link
Member

Choose a reason for hiding this comment

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

@davidcui-amzn can you take a look, and if it's safe to remove this, let's merge in the PR first

Copy link
Contributor

Choose a reason for hiding this comment

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

Can confirm, navigation and notifications are not used. Please remove them instead of commenting out, as Zhongnan suggests above.

@@ -13,13 +13,16 @@
* permissions and limitations under the License.
*/

import { NavigationPublicPluginStart } from '../../../src/plugins/navigation/public';
// import { NavigationPublicPluginStart } from '../../../src/plugins/navigation/public';
Copy link
Member

Choose a reason for hiding this comment

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

same here


// eslint-disable-next-line @typescript-eslint/no-empty-interface
export interface OpendistroKibanaReportsPluginStart {}

export interface AppPluginStartDependencies {
navigation: NavigationPublicPluginStart;
// navigation: NavigationPublicPluginStart;
Copy link
Member

Choose a reason for hiding this comment

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

here

{ notifications, http, chrome }: CoreStart,
{ navigation }: AppPluginStartDependencies,
{ http, chrome, uiSettings }: CoreStart,
{ }: AppPluginStartDependencies,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to data here?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, the parameter for AppPluginStartDependencies was navigation previously, which is not used

@zhongnansu zhongnansu added the need migrate need migrate to OpenSearch label May 11, 2021
@iget-master
Copy link
Author

I'm moving this PR to https://github.com/opensearch-project/dashboards-reports with the requested changes. I think this repository will be cloded, right?

@zhongnansu
Copy link
Member

Moved to opensearch-project/reporting#49

@zhongnansu zhongnansu closed this May 25, 2021
@zhongnansu zhongnansu removed the need migrate need migrate to OpenSearch label Jun 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants