You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/10/20 16:29:08 UTC
[GitHub] [ozone] smitajoshi12 opened a new pull request, #3868: HDDS-7323. Recon: Auto refresh toggle is switched back when visiting new site
smitajoshi12 opened a new pull request, #3868:
URL: https://github.com/apache/ozone/pull/3868
What changes were proposed in this pull request?
Auto Reload toggle on and off should be in sync with Datanode, Pipeline and Overview Page.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7323
## How was this patch tested?
Manually
--
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
[GitHub] [ozone] dombizita commented on a diff in pull request #3868: HDDS-7323. Recon: Auto refresh toggle is switched back when visiting new site
Posted by GitBox <gi...@apache.org>.
dombizita commented on code in PR #3868:
URL: https://github.com/apache/ozone/pull/3868#discussion_r1007890611
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/utils/autoReloadHelper.tsx:
##########
@@ -49,6 +49,10 @@ class AutoReloadHelper {
this.stopPolling();
}
};
+
+ toggleChecking = (checked: boolean) => {
+ sessionStorage.setItem('toggleCheck', JSON.stringify(checked));
+ };
Review Comment:
Ohh I think I misunderstood you question @DaveTeng0, I thought you are asking about the change I suggested, sorry :) `toggleCheck` will be the property we set in the [sessionStorage](https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage), so in the browser we can ask for that property until the page session ends (if we would set it in the [localStorage](https://developer.mozilla.org/en-US/docs/Web/API/Window/localStorage) that would have no expiration time). The `checked` is a variable in the code, which stores the current state of the toggle in the Recon UI. That is why in the new method we set the `toggleCheck` to `checked`, which is passed as a parameter to the method (same parameter is used in the already existing `handleAutoReloadToggle` method). I hope this answers your question :) correct me if I am wrong @smitajoshi12
--
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
[GitHub] [ozone] dombizita commented on a diff in pull request #3868: HDDS-7323. Recon: Auto refresh toggle is switched back when visiting new site
Posted by GitBox <gi...@apache.org>.
dombizita commented on code in PR #3868:
URL: https://github.com/apache/ozone/pull/3868#discussion_r1011736077
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/utils/autoReloadHelper.tsx:
##########
@@ -49,6 +49,10 @@ class AutoReloadHelper {
this.stopPolling();
}
};
+
+ toggleChecking = (checked: boolean) => {
+ sessionStorage.setItem('toggleCheck', JSON.stringify(checked));
+ };
Review Comment:
What we could do is to name the property that is set in the sessionStorage to `checked`, but as we have this information in the browser it is better to have a name that is easy to understand what it stands for.
--
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
[GitHub] [ozone] kerneltime commented on pull request #3868: HDDS-7323. Recon: Auto refresh toggle is switched back when visiting new site
Posted by GitBox <gi...@apache.org>.
kerneltime commented on PR #3868:
URL: https://github.com/apache/ozone/pull/3868#issuecomment-1289280582
cc @duongkame
--
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
[GitHub] [ozone] DaveTeng0 commented on a diff in pull request #3868: HDDS-7323. Recon: Auto refresh toggle is switched back when visiting new site
Posted by GitBox <gi...@apache.org>.
DaveTeng0 commented on code in PR #3868:
URL: https://github.com/apache/ozone/pull/3868#discussion_r1008308107
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/utils/autoReloadHelper.tsx:
##########
@@ -49,6 +49,10 @@ class AutoReloadHelper {
this.stopPolling();
}
};
+
+ toggleChecking = (checked: boolean) => {
+ sessionStorage.setItem('toggleCheck', JSON.stringify(checked));
+ };
Review Comment:
Make sense! So, is it possible that we could use the var `checked` in sessionStorage with its original name, instead of creating a new var `toggleCheck` ?
--
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
[GitHub] [ozone] kerneltime commented on pull request #3868: HDDS-7323. Recon: Auto refresh toggle is switched back when visiting new site
Posted by GitBox <gi...@apache.org>.
kerneltime commented on PR #3868:
URL: https://github.com/apache/ozone/pull/3868#issuecomment-1289280252
@dombizita
--
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
[GitHub] [ozone] dombizita commented on pull request #3868: HDDS-7323. Recon: Auto refresh toggle is switched back when visiting new site
Posted by GitBox <gi...@apache.org>.
dombizita commented on PR #3868:
URL: https://github.com/apache/ozone/pull/3868#issuecomment-1303485897
Thanks for updating your patch @smitajoshi12, it looks good to me!
--
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
[GitHub] [ozone] smengcl merged pull request #3868: HDDS-7323. Recon: Auto refresh toggle is switched back when visiting new site
Posted by GitBox <gi...@apache.org>.
smengcl merged PR #3868:
URL: https://github.com/apache/ozone/pull/3868
--
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
[GitHub] [ozone] smengcl commented on a diff in pull request #3868: HDDS-7323. Recon: Auto refresh toggle is switched back when visiting new site
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3868:
URL: https://github.com/apache/ozone/pull/3868#discussion_r1016903032
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/components/autoReloadPanel/autoReloadPanel.tsx:
##########
@@ -79,7 +80,7 @@ class AutoReloadPanel extends React.Component<IAutoReloadPanelProps> {
return (
<div className='auto-reload-panel'>
Auto Refresh
- <Switch defaultChecked size='small' className='toggle-switch' onChange={this.autoReloadToggleHandler}/>
+ <Switch defaultChecked={toggleRefreshCheck} size='small' className='toggle-switch' onChange={this.autoReloadToggleHandler}/>
Review Comment:
```suggestion
<Switch defaultChecked={autoReloadEnabled} size='small' className='toggle-switch' onChange={this.autoReloadToggleHandler}/>
```
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/components/autoReloadPanel/autoReloadPanel.tsx:
##########
@@ -41,6 +41,7 @@ class AutoReloadPanel extends React.Component<IAutoReloadPanelProps> {
render() {
const {onReload, lastRefreshed, lastUpdatedOMDBDelta, lastUpdatedOMDBFull, isLoading} = this.props;
+ const toggleRefreshCheck = sessionStorage.getItem('toggleRefreshCheck') === 'false' ? false : true;
Review Comment:
```suggestion
const autoReloadEnabled = sessionStorage.getItem('autoReloadEnabled') === 'false' ? false : true;
```
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/utils/autoReloadHelper.tsx:
##########
@@ -43,6 +43,7 @@ class AutoReloadHelper {
};
handleAutoReloadToggle = (checked: boolean) => {
+ sessionStorage.setItem('toggleRefreshCheck', JSON.stringify(checked));
Review Comment:
```suggestion
sessionStorage.setItem('autoReloadEnabled', JSON.stringify(checked));
```
--
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
[GitHub] [ozone] dombizita commented on a diff in pull request #3868: HDDS-7323. Recon: Auto refresh toggle is switched back when visiting new site
Posted by GitBox <gi...@apache.org>.
dombizita commented on code in PR #3868:
URL: https://github.com/apache/ozone/pull/3868#discussion_r1006621845
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/utils/autoReloadHelper.tsx:
##########
@@ -49,6 +49,10 @@ class AutoReloadHelper {
this.stopPolling();
}
};
+
+ toggleChecking = (checked: boolean) => {
+ sessionStorage.setItem('toggleCheck', JSON.stringify(checked));
+ };
Review Comment:
Couldn't we move this `sessionStorage.setItem('toggleCheck', JSON.stringify(checked));` line into the already existing `handleAutoReloadToggle` method? For example to the first line of the method, before the conditions. If we do it this way we wouldn't need a new `toggleChecking` method.
--
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
[GitHub] [ozone] dombizita commented on a diff in pull request #3868: HDDS-7323. Recon: Auto refresh toggle is switched back when visiting new site
Posted by GitBox <gi...@apache.org>.
dombizita commented on code in PR #3868:
URL: https://github.com/apache/ozone/pull/3868#discussion_r1007883287
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/utils/autoReloadHelper.tsx:
##########
@@ -49,6 +49,10 @@ class AutoReloadHelper {
this.stopPolling();
}
};
+
+ toggleChecking = (checked: boolean) => {
+ sessionStorage.setItem('toggleCheck', JSON.stringify(checked));
+ };
Review Comment:
There is basically no difference, I just thought that with the approach I suggested we can avoid adding and passing another method between the pages with the same parameter. Also I think this one line that is in the method fits in the already existing one, the naming isn't confusing either, the functionality of `handleAutoReloadToggle` should be handling that toggle, so that storage setting is okay there I think.
--
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
[GitHub] [ozone] DaveTeng0 commented on a diff in pull request #3868: HDDS-7323. Recon: Auto refresh toggle is switched back when visiting new site
Posted by GitBox <gi...@apache.org>.
DaveTeng0 commented on code in PR #3868:
URL: https://github.com/apache/ozone/pull/3868#discussion_r1007545589
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/utils/autoReloadHelper.tsx:
##########
@@ -49,6 +49,10 @@ class AutoReloadHelper {
this.stopPolling();
}
};
+
+ toggleChecking = (checked: boolean) => {
+ sessionStorage.setItem('toggleCheck', JSON.stringify(checked));
+ };
Review Comment:
hmm.. for myself learning, what's the difference between _checked_ in method _handleAutoReloadToggle_, and _toggleCheck_ in new method _toggleChecking_?
only type difference??
--
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