dombizita commented on code in PR #5176:
URL: https://github.com/apache/ozone/pull/5176#discussion_r1305648227
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/components/overviewCard/overviewCard.tsx:
##########
@@ -46,18 +46,31 @@ const defaultProps = {
interface IOverviewCardWrapperProps {
linkToUrl: string;
+ title:string
Review Comment:
nit
```suggestion
title: string
```
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/insights/om/om.tsx:
##########
@@ -444,15 +443,25 @@ export class Om extends React.Component<Record<string,
object>, IOmdbInsightsSta
};
componentDidMount(): void {
- // Fetch mismatch containers on component mount
- this.fetchMismatchContainers(this.state.DEFAULT_LIMIT,
this.state.prevKeyMismatch, this.state.mismatchMissingState);
+ if (this.state.activeTab === '2') {
+ this.fetchOpenKeys(this.state.includeFso, this.state.includeNonFso,
this.state.DEFAULT_LIMIT, this.state.prevKeyOpen);
+ } else if (this.state.activeTab === '3') {
+ keysPendingExpanded =[];
+ this.fetchDeletePendingKeys(this.state.DEFAULT_LIMIT,
this.state.prevKeyDeletePending);
+ } else if (this.state.activeTab === '4') {
+ this.fetchDeletedKeys(this.state.DEFAULT_LIMIT,
this.state.prevKeyDeleted);
+ }
+ else {
+ this.fetchMismatchContainers(this.state.DEFAULT_LIMIT,
this.state.prevKeyMismatch, this.state.mismatchMissingState);
+ }
};
fetchMismatchContainers = (limit: number, prevKeyMismatch: number,
mismatchMissingState: any) => {
this.setState({
loading: true,
- clickable: true,
- prevClickable: true
+ nextClickable: true,
+ prevClickable: true,
+ mismatchMissingState
Review Comment:
can you help me understand why is this needed? I don't see any new changes
related to this, was something not working correctly before?
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/insights/om/om.tsx:
##########
@@ -493,13 +502,15 @@ export class Om extends React.Component<Record<string,
object>, IOmdbInsightsSta
fetchOpenKeys = (includeFso: boolean, includeNonFso: boolean, limit: number,
prevKeyOpen: string) => {
this.setState({
loading: true,
- clickable: true,
- prevClickable:true
+ nextClickable: true,
+ prevClickable: true,
+ includeFso,
+ includeNonFso
Review Comment:
can you help me understand why is this needed? I don't see any new changes
related to this, was something not working correctly before?
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/insights/om/om.tsx:
##########
@@ -326,12 +326,12 @@ export class Om extends React.Component<Record<string,
object>, IOmdbInsightsSta
prevKeyDeletePending: "",
prevKeyDeleted: 0,
expandedRowData: {},
- activeTab: '1',
+ activeTab: props.location.state && props.location.state.activeTab !==
'1' ? props.location.state.activeTab:'1',
Review Comment:
can't we just simply use `props.location.state.activeTab` here if the
`props.location.state` is set, otherwise return `'1'`?
```suggestion
activeTab: props.location.state ? props.location.state.activeTab : '1',
```
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/components/overviewCard/overviewCard.tsx:
##########
@@ -46,18 +46,31 @@ const defaultProps = {
interface IOverviewCardWrapperProps {
linkToUrl: string;
+ title:string
}
class OverviewCardWrapper extends React.Component<IOverviewCardWrapperProps> {
render() {
const {linkToUrl, children} = this.props;
- if (linkToUrl) {
+ if (linkToUrl ) {
+ if(linkToUrl === '/Om'){
Review Comment:
nit
```suggestion
if (linkToUrl) {
if (linkToUrl === '/Om'){
```
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/insights/om/om.tsx:
##########
@@ -444,15 +443,25 @@ export class Om extends React.Component<Record<string,
object>, IOmdbInsightsSta
};
componentDidMount(): void {
- // Fetch mismatch containers on component mount
- this.fetchMismatchContainers(this.state.DEFAULT_LIMIT,
this.state.prevKeyMismatch, this.state.mismatchMissingState);
+ if (this.state.activeTab === '2') {
+ this.fetchOpenKeys(this.state.includeFso, this.state.includeNonFso,
this.state.DEFAULT_LIMIT, this.state.prevKeyOpen);
+ } else if (this.state.activeTab === '3') {
+ keysPendingExpanded =[];
+ this.fetchDeletePendingKeys(this.state.DEFAULT_LIMIT,
this.state.prevKeyDeletePending);
+ } else if (this.state.activeTab === '4') {
+ this.fetchDeletedKeys(this.state.DEFAULT_LIMIT,
this.state.prevKeyDeleted);
+ }
+ else {
+ this.fetchMismatchContainers(this.state.DEFAULT_LIMIT,
this.state.prevKeyMismatch, this.state.mismatchMissingState);
+ }
Review Comment:
or is it possible, that the `this.state.activeTab` is not set here? can it
be different from 1, 2, 3 or 4 (if we have 4 tabs)?
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/insights/om/om.tsx:
##########
@@ -444,15 +443,25 @@ export class Om extends React.Component<Record<string,
object>, IOmdbInsightsSta
};
componentDidMount(): void {
- // Fetch mismatch containers on component mount
- this.fetchMismatchContainers(this.state.DEFAULT_LIMIT,
this.state.prevKeyMismatch, this.state.mismatchMissingState);
+ if (this.state.activeTab === '2') {
+ this.fetchOpenKeys(this.state.includeFso, this.state.includeNonFso,
this.state.DEFAULT_LIMIT, this.state.prevKeyOpen);
+ } else if (this.state.activeTab === '3') {
+ keysPendingExpanded =[];
+ this.fetchDeletePendingKeys(this.state.DEFAULT_LIMIT,
this.state.prevKeyDeletePending);
+ } else if (this.state.activeTab === '4') {
+ this.fetchDeletedKeys(this.state.DEFAULT_LIMIT,
this.state.prevKeyDeleted);
+ }
+ else {
+ this.fetchMismatchContainers(this.state.DEFAULT_LIMIT,
this.state.prevKeyMismatch, this.state.mismatchMissingState);
+ }
Review Comment:
I think we shouldn't do this in an `else` statement, rather in an `else if`
where you are checking if the active tab is `'1'`. let me know what you think
of this!
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/components/overviewCard/overviewCard.tsx:
##########
@@ -46,18 +46,31 @@ const defaultProps = {
interface IOverviewCardWrapperProps {
linkToUrl: string;
+ title:string
}
class OverviewCardWrapper extends React.Component<IOverviewCardWrapperProps> {
render() {
const {linkToUrl, children} = this.props;
- if (linkToUrl) {
+ if (linkToUrl ) {
+ if(linkToUrl === '/Om'){
return (
- <Link to={linkToUrl}>
+ <Link to={{
+ pathname: linkToUrl,
+ state: { activeTab: children._owner &&
children._owner.stateNode.props && children._owner.stateNode.props.title ===
"Open Keys Summary"? '2':'3'}
Review Comment:
I understood that the open keys and pending deleted keys summary are both
tabs on the OM Insight page, so somehow we need to set the active tab, but
right now if we add another card with a link to `/Om` to the Overview page, we
will need to modify this, otherwise it will take us to the pending deleted keys
summary tab.
just to think ahead and to make the code more readable can we set the active
tab based on the card title previous to this? with that we don't need an if
statement here.
let me know what you think of this idea!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
