-
Notifications
You must be signed in to change notification settings - Fork 100
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
Added Metrics Home Page and date picker #1125
Added Metrics Home Page and date picker #1125
Conversation
Signed-off-by: Sean Li <lnse@amazon.com>
Codecov Report
@@ Coverage Diff @@
## feature/metrics opensearch-project/observability#1125 +/- ##
=====================================================
+ Coverage 53.12% 53.53% +0.40%
Complexity 291 291
=====================================================
Files 280 279 -1
Lines 9428 9439 +11
Branches 2208 2209 +1
=====================================================
+ Hits 5009 5053 +44
+ Misses 4249 4216 -33
Partials 170 170
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
minor changes
Signed-off-by: Sean Li <lnse@amazon.com>
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.
Ideas for next code-change.
Feel free to reply with questions or discussion.
return returnTime; | ||
}; | ||
|
||
export const onTimeChange = ( |
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.
HI Sean. This is an after-the-fact review, to keep in mind for next time.
A few points are coming to mind for this Fn.
- The new component (metrics/index) seems to be the singular use for this Fn. I think this might be an exercise in premature optimization. I would rather we avoid separating supporting code from it's use until we have a compelling case for it's re-use. We also want to be careful about separating callback-Fns from their use-case. This is a non-pure Fn. with its use of setRCU, setStart, setEnd, so side-effects are many. This would also be hard to unit-test, because there is little we can do to test the "effect" of setRCU, setStart, setEnd beyond just ensuring they are "called", which is quite a weak test -- and also brittle - a test of this Fn. could not endure any refactoring at all.
Thanks for considering these points in your next code-change.
}; | ||
|
||
const onDatePickerChange = (props: OnTimeChangeProps) => { | ||
onTimeChange( |
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.
if onTimeChange were included in this component's code, it's usage would be clear, you would be able to reduce the args of the Fn (setRCU, setStart, setEnd are in-context). This callback (onDatePickerChange) could have been reduced to :
(props) => {
onTimeChange(props);
onRefreshFilters(props);
}
onRefreshFilters would need a simple refactor to destructure the props instead of taking args-list.
const [dateDisabled, setDateDisabled] = useState(false); | ||
|
||
const onRefreshFilters = (startTime: ShortDate, endTime: ShortDate) => { | ||
// if (!isDateValid(convertDateTime(startTime), convertDateTime(endTime, false), setToast)) { |
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.
Do we need these commented out code?
* Added Metrics Home Page and date picker Signed-off-by: Sean Li <lnse@amazon.com> * Minor changes Signed-off-by: Sean Li <lnse@amazon.com> Signed-off-by: Sean Li <lnse@amazon.com>
* Added Metrics Home Page and date picker Signed-off-by: Sean Li <lnse@amazon.com> * Minor changes Signed-off-by: Sean Li <lnse@amazon.com> Signed-off-by: Sean Li <lnse@amazon.com>
Signed-off-by: Sean Li lnse@amazon.com
Description
Adds Metrics Home Page to Observability side bar and date picker for metrics
Issues Resolved
opensearch-project/dashboards-observability#60
opensearch-project/dashboards-observability#61
Check List
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.