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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from

Conversation

iget-master
Copy link

Description

  • 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
  • Implement tests to check if uiSettings is being called correctly
  • Update tests to add missing new prop uiSettings

Issues Resolved

#41

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@iget-master iget-master mentioned this pull request May 14, 2021
Closed
@iget-master
Copy link
Author

Note that although this pull request fixes the Issue #41, this plugin currently breaks the usage of relative dates (date math), so the issue opendistro-for-elasticsearch/kibana-reports#353 (still on the old repository, asked to @zhongnansu move it) must be fixed to this PR make sense.

@zhongnansu zhongnansu mentioned this pull request May 25, 2021
Closed
@iget-master
Copy link
Author

Can someone give some attention here to merge this fix out? I'm pretending to fully migrate to opensearch but this is preventing me from doing it.

@zhongnansu
Copy link
Member

Note that although this pull request fixes the Issue #41, this plugin currently breaks the usage of relative dates (date math), so the issue opendistro-for-elasticsearch/kibana-reports#353 (still on the old repository, asked to @zhongnansu move it) must be fixed to this PR make sense.

opendistro-for-elasticsearch/kibana-reports#353

@iget-master sorry for not able to follow up in time. The opendistro-for-elasticsearch/kibana-reports#353 has been fixed recently. We can move on with this PR.

@joshuali925 Josh, could you guide him for this PR? Since we recently added uisettings for date_format, does he need to do some extra work to be in consistent with your change? If not, we just ask him to resolve the conflicts and we'll review the PR

joshuali925
joshuali925 reviewed Nov 2, 2021
View reviewed changes
dashboards-reports/public/components/report_definitions/report_settings/time_range.tsx
@@ -203,6 +203,15 @@ export function TimeRangeSelect(props) {
setIsLoading(false);
};

const commonlyUsedRanges = uiSettings!
Copy link
Member

Choose a reason for hiding this comment

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

@iget-master thanks for the PR. We recently added support for dateFormat in uiSettings. There is a module (not a prop since some code will run in discover) to expose the ui settings client https://github.com/opensearch-project/dashboards-reports/blob/93d885c641c8c7fc8beeb081bee6ffd30b44f73b/dashboards-reports/public/components/utils/settings_service.ts#L16-L23

I believe the only change needed is here, and uiSettings can be called this way
https://github.com/opensearch-project/dashboards-reports/blob/93d885c641c8c7fc8beeb081bee6ffd30b44f73b/dashboards-reports/public/components/main/main_utils.tsx#L199

Could you update the PR please? thanks

Copy link
Member

Choose a reason for hiding this comment

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

@iget-master any thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

I think that's ok. I'm a little busy next days, but asap i can update the pr

@anirudha anirudha added enhancement New feature or request feature request good first issue Good for newcomers labels Nov 16, 2021
@zhongnansu zhongnansu removed the good first issue Good for newcomers label Nov 19, 2021
@anirudha
Copy link
Collaborator

closing an older PR no coverage

@anirudha anirudha closed this Oct 10, 2022
kavilla pushed a commit to kavilla/dashboards-reports that referenced this pull request Jul 12, 2023
@opensearch-trigger-bot @rupal-bq
Update yarn dependencies (opensearch-project#48) (opensearch-project#49)
50598a2 
Signed-off-by: Rupal Mahajan <maharup@amazon.com>
(cherry picked from commit 9ce6c2576432e33bf9e570d96eafe848d8ef705b)

Co-authored-by: Rupal Mahajan <maharup@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhongnansu zhongnansu zhongnansu left review comments

@joshuali925 joshuali925 joshuali925 left review comments

Assignees

@joshuali925 joshuali925

@zhongnansu zhongnansu

Labels
enhancement New feature or request feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@iget-master @zhongnansu @anirudha @joshuali925 @iget-esoares