-
Notifications
You must be signed in to change notification settings - Fork 339
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
[CDAP-12849][CDAP-12869] Ops Dashboard Navigation #9979
Conversation
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.
Just a few minor comments. Rest of code LGTM 👍
import PropTypes from 'prop-types'; | ||
import {getCurrentNamespace} from 'services/NamespaceStore'; | ||
|
||
function NamespacesPickerView() { |
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.
nit: why not just export default function NamespacesPickerView()
?
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.
there will be an expansion to this, it will be a connected component, it's just unfinished at the moment
@@ -98,6 +106,36 @@ export function renderGraph(selector, containerWidth, containerHeight, data, leg | |||
.select('line') | |||
.remove(); | |||
|
|||
let dateMap = {}; | |||
data.forEach((d) => { | |||
let time = parseInt(d.time); |
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.
nit: parseInt(d.time, 10)
|
||
start = parseInt(start, 10); | ||
duration = 1440; |
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.
nit: unnecessary, since you already set default duration = 1440
in the params?
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.
Couple minor comments
.toggle-all-dropdown { | ||
.dropdown-toggle-btn { | ||
color: initial; | ||
background-color: initial; |
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.
nit: I think @ajainarayanan once mentioned that we shouldn't use initial
values anymore, since the browser default color might be different for each browser. Maybe we can just straight away use the right color here.
|
||
&:hover { | ||
color: initial; | ||
background-color: initial; |
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.
same here too
import Popover from 'components/Popover'; | ||
import NamespaceStore, {getCurrentNamespace} from 'services/NamespaceStore'; | ||
import IconSVG from 'components/IconSVG'; | ||
import {setNewNamespaces} from 'components/OpsDashboard/store/ActionCreator'; |
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.
nit: name this setNewNamespacesPick
to make it more clear?
</div> | ||
); | ||
} else { | ||
|
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.
nit: extra white space
{monitorTitle} | ||
|
||
<div className="monitor-more text-xs-right"> | ||
<div className="d-inline-block"> |
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.
is this class name a typo?
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.
no, this is bootstrap utility class to set it to inline-block
Code LGTM 👍 |
8425745
to
6e3fb9b
Compare
JIRAs: