You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "dombizita (via GitHub)" <gi...@apache.org> on 2023/08/28 08:18:45 UTC
[GitHub] [ozone] dombizita commented on a diff in pull request #5176: HDDS-9076. Recon UI OMDB Insights Changes and Improvements
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org