You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/05/03 19:29:58 UTC
[GitHub] [accumulo] DomGarguilo opened a new pull request, #2674: Bug fix for caption in tservers table in monitor
DomGarguilo opened a new pull request, #2674:
URL: https://github.com/apache/accumulo/pull/2674
A bug was found in the feature added in #2663 where the caption explaining the highlighted rows would be visible in certain scenarios when there were no highlighted rows in the table. The caption is only meant to be visible when there are highlighted rows. The changes in this PR make it so that the caption is hidden by default and only shown when a row is actually highlighted.
Additionally, I renamed the variable representing the tservers table to make it more clear.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a diff in pull request #2674: Bug fix for caption in tservers table in monitor
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2674:
URL: https://github.com/apache/accumulo/pull/2674#discussion_r864724654
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/tservers.js:
##########
@@ -18,29 +18,25 @@
*/
"use strict";
-var tserversList;
+var tserversTable;
/**
* Creates tservers initial table
*/
$(document).ready(function() {
-
+
+ // hide the note about highlighted rows by default
+ $('#recovery-caption').hide();
Review Comment:
Should be able to hide this on the FTL file. If you do it here, every time the page loads, you may see it flash on the screen. The Manager page (manager.ftl) has an alert that is hidden by default using `style="display: none;"`
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2674: Bug fix for caption in tservers table in monitor
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2674:
URL: https://github.com/apache/accumulo/pull/2674#discussion_r864883876
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/tservers.js:
##########
@@ -18,29 +18,25 @@
*/
"use strict";
-var tserversList;
+var tserversTable;
/**
* Creates tservers initial table
*/
$(document).ready(function() {
-
+
+ // hide the note about highlighted rows by default
+ $('#recovery-caption').hide();
Review Comment:
This is how the tables are handled too. On refresh you can see the non functioning servers table and the dead servers table flash on the screen before going away.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a diff in pull request #2674: Bug fix for caption in tservers table in monitor
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2674:
URL: https://github.com/apache/accumulo/pull/2674#discussion_r864727573
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/tservers.js:
##########
@@ -92,10 +88,13 @@ $(document).ready(function() {
// reset background of each row
$(row).css('background-color', '');
- // return if current rows tserver is not recovering
+ // return if the current rows' tserver is not recovering
Review Comment:
I am bad with grammer but I think `row's` is what you want here...
"The general rule is that the possessive of a singular noun is formed by adding an apostrophe and s, whether the singular noun ends in s or not." i.e. `the child’s toy`
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2674: Bug fix for caption in tservers table in monitor
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2674:
URL: https://github.com/apache/accumulo/pull/2674#discussion_r865074838
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/tservers.js:
##########
@@ -18,29 +18,25 @@
*/
"use strict";
-var tserversList;
+var tserversTable;
/**
* Creates tservers initial table
*/
$(document).ready(function() {
-
+
+ // hide the note about highlighted rows by default
+ $('#recovery-caption').hide();
Review Comment:
addressed in [05e9e7a](https://github.com/apache/accumulo/pull/2674/commits/05e9e7a2b2d6bc391fc5003652cdec61132545bb)
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo merged pull request #2674: Bug fix for caption in tservers table in monitor
Posted by GitBox <gi...@apache.org>.
DomGarguilo merged PR #2674:
URL: https://github.com/apache/accumulo/pull/2674
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on pull request #2674: Bug fix for caption in tservers table in monitor
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on PR #2674:
URL: https://github.com/apache/accumulo/pull/2674#issuecomment-1117601451
The changes in [05e9e7a](https://github.com/apache/accumulo/pull/2674/commits/05e9e7a2b2d6bc391fc5003652cdec61132545bb) change a few things:
* Now, the dead server table and the non-functioning server tables are hidden by default via inline styling in the .ftl file which removes the flashing of all the tables when the page is manually refreshed
* The conditions that will hide/show a table are assessed on each reload so that the page will update when the auto-refresh feature is enabled
* A function was added to refresh the recovery list which is used to highlight the rows and determine if the corresponding caption should be displayed
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2674: Bug fix for caption in tservers table in monitor
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2674:
URL: https://github.com/apache/accumulo/pull/2674#discussion_r864813357
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/tservers.js:
##########
@@ -18,29 +18,25 @@
*/
"use strict";
-var tserversList;
+var tserversTable;
/**
* Creates tservers initial table
*/
$(document).ready(function() {
-
+
+ // hide the note about highlighted rows by default
+ $('#recovery-caption').hide();
Review Comment:
I'll try that out but I was under the impression that it needs to happen here because this code will run again when the table is refreshed which will clear the caption if needed. I think if it were in the FTL it might never be hidden again once it is shown.
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/tservers.js:
##########
@@ -92,10 +88,13 @@ $(document).ready(function() {
// reset background of each row
$(row).css('background-color', '');
- // return if current rows tserver is not recovering
+ // return if the current rows' tserver is not recovering
Review Comment:
Whoops, yea I think you are correct.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2674: Bug fix for caption in tservers table in monitor
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2674:
URL: https://github.com/apache/accumulo/pull/2674#discussion_r865040928
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/tservers.js:
##########
@@ -18,29 +18,25 @@
*/
"use strict";
-var tserversList;
+var tserversTable;
/**
* Creates tservers initial table
*/
$(document).ready(function() {
-
+
+ // hide the note about highlighted rows by default
+ $('#recovery-caption').hide();
Review Comment:
You can avoid the flash by having the initial style be hidden, and then `.show()` and `.hide()` it later, as circumstances require.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org