You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/11/30 03:25:40 UTC
[GitHub] [nifi] mtien-apache opened a new pull request #5559: NIFI-9336 - Show icon for property values with leading or trailing whitespace
mtien-apache opened a new pull request #5559:
URL: https://github.com/apache/nifi/pull/5559
<!--
Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements. See the NOTICE file distributed with
this work for additional information regarding copyright ownership.
The ASF licenses this file to You under the Apache License, Version 2.0
(the "License"); you may not use this file except in compliance with
the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
Jira: https://issues.apache.org/jira/browse/NIFI-9336
When configuring processors and controller services in the UI, if a user enters a property value that has leading or trailing whitespace, it's not always obvious and may not be desired. To indicate when a value contains leading/trailing whitespace, an icon with a tooltip will show next to the value in processor and controller services configurations.
To test:
- go to configurations in a processor or controller service
- enter a property value with leading and/or trailing whitespace
- you should see an info icon and tooltip show to the right of the value
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [x] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [x] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [x] Have you verified that the full build is successful on JDK 8?
- [ ] Have you verified that the full build is successful on JDK 11?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.
--
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@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mcgilman commented on a change in pull request #5559: NIFI-9336 - Show icon for property values with leading or trailing whitespace
Posted by GitBox <gi...@apache.org>.
mcgilman commented on a change in pull request #5559:
URL: https://github.com/apache/nifi/pull/5559#discussion_r759531704
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/jquery/propertytable/jquery.propertytable.js
##########
@@ -1763,13 +1771,26 @@
var propertyHistory = history[property];
// format the tooltip
- var tooltip = nfCommon.formatPropertyTooltip(propertyDescriptor, propertyHistory);
+ var propertyTooltip = nfCommon.formatPropertyTooltip(propertyDescriptor, propertyHistory);
- if (nfCommon.isDefinedAndNotNull(tooltip)) {
+ if (nfCommon.isDefinedAndNotNull(propertyTooltip)) {
infoIcon.qtip($.extend({},
nfCommon.config.tooltipConfig,
{
- content: tooltip
+ content: propertyTooltip
+ }));
+ }
+ }
+
+ var whitespaceIcon = $(this).find('div.fa-info');
+ if (whitespaceIcon.length && !whitespaceIcon.data('qtip')) {
+ var whitespaceTooltip = nfCommon.formatWhitespaceTooltip();
+
+ if (nfCommon.isDefinedAndNotNull(whitespaceTooltip)) {
+ whitespaceIcon.qtip($.extend({},
Review comment:
Tooltips from qtip should be manually cleaned up to avoid a dom leak. If you open up DevTools and show one of these new tooltips, close the configuration dialog, open the configuration dialog, and show another one of these new tooltips you can see at the bottom of the dom elements we continually add new qtip content.
In this case, you can clean up by updating the `clear` function to also clean up `div.fa-info`.
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/nf-common.js
##########
@@ -996,6 +996,30 @@
}
},
+ /**
+ * Returns a tooltip for leading and/or trailing whitespace.
+ *
+ * @returns {string}
+ */
+ formatWhitespaceTooltip: function () {
+ return nfCommon.escapeHtml('The specified value contains leading and/or trailing whitespace character(s). ' +
+ 'This could produce unexpected results if it was not intentional.');
+
+ },
+
+ /**
+ * Checks the specified value for leading and/or trailing whitespace.
+ *
+ * @argument {string} value The value to check
+ */
+ hasWhitespace : function (value) {
+ if (!value) {
+ return false;
+ }
+ var leadOrTrailWhitespaceRegex = /^[ \s]+|[ \s]+$/;
Review comment:
If a property value only contains whitespace, this regex will hit. Only whitespace would be considered a normal occurrence as we support delimiters for example. I think this logic needs to be updated to only trigger when the value contains leading or trailing whitespace but not only whitespace.
--
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@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mcgilman commented on a change in pull request #5559: NIFI-9336 - Show icon for property values with leading or trailing whitespace
Posted by GitBox <gi...@apache.org>.
mcgilman commented on a change in pull request #5559:
URL: https://github.com/apache/nifi/pull/5559#discussion_r759531704
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/jquery/propertytable/jquery.propertytable.js
##########
@@ -1763,13 +1771,26 @@
var propertyHistory = history[property];
// format the tooltip
- var tooltip = nfCommon.formatPropertyTooltip(propertyDescriptor, propertyHistory);
+ var propertyTooltip = nfCommon.formatPropertyTooltip(propertyDescriptor, propertyHistory);
- if (nfCommon.isDefinedAndNotNull(tooltip)) {
+ if (nfCommon.isDefinedAndNotNull(propertyTooltip)) {
infoIcon.qtip($.extend({},
nfCommon.config.tooltipConfig,
{
- content: tooltip
+ content: propertyTooltip
+ }));
+ }
+ }
+
+ var whitespaceIcon = $(this).find('div.fa-info');
+ if (whitespaceIcon.length && !whitespaceIcon.data('qtip')) {
+ var whitespaceTooltip = nfCommon.formatWhitespaceTooltip();
+
+ if (nfCommon.isDefinedAndNotNull(whitespaceTooltip)) {
+ whitespaceIcon.qtip($.extend({},
Review comment:
Tooltips from qtip should be manually cleaned up to avoid a dom leak. If you open up DevTools and show one of these new tooltips, close the configuration dialog, open the configuration dialog, and show another one of these new tooltips you can see at the bottom of the dom elements we continually add new qtip content.
In this case, you can address these by updating the `clear` function to also clean up `div.fa-info`.
--
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@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mtien-apache commented on pull request #5559: NIFI-9336 - Show icon for property values with leading or trailing whitespace
Posted by GitBox <gi...@apache.org>.
mtien-apache commented on pull request #5559:
URL: https://github.com/apache/nifi/pull/5559#issuecomment-984815961
@mcgilman Per our offline discussion, I've pushed a commit that fixes a bug to clean up tooltips. With the addition of the whitespace icon, when editing a value cell, it was re-rendering the property table, causing property tooltips to become orphaned on the DOM. This no longer occurs. Can you check again? Thanks!
--
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@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mcgilman commented on pull request #5559: NIFI-9336 - Show icon for property values with leading or trailing whitespace
Posted by GitBox <gi...@apache.org>.
mcgilman commented on pull request #5559:
URL: https://github.com/apache/nifi/pull/5559#issuecomment-982652584
Thanks for the PR @mtien-apache! Will review...
--
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@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mtien-apache commented on a change in pull request #5559: NIFI-9336 - Show icon for property values with leading or trailing whitespace
Posted by GitBox <gi...@apache.org>.
mtien-apache commented on a change in pull request #5559:
URL: https://github.com/apache/nifi/pull/5559#discussion_r759771041
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/nf-common.js
##########
@@ -996,6 +996,30 @@
}
},
+ /**
+ * Returns a tooltip for leading and/or trailing whitespace.
+ *
+ * @returns {string}
+ */
+ formatWhitespaceTooltip: function () {
+ return nfCommon.escapeHtml('The specified value contains leading and/or trailing whitespace character(s). ' +
+ 'This could produce unexpected results if it was not intentional.');
+
+ },
+
+ /**
+ * Checks the specified value for leading and/or trailing whitespace.
+ *
+ * @argument {string} value The value to check
+ */
+ hasWhitespace : function (value) {
+ if (!value) {
+ return false;
+ }
+ var leadOrTrailWhitespaceRegex = /^[ \s]+|[ \s]+$/;
Review comment:
Thanks for reviewing, @mcgilman! I misunderstood the criteria. I've changed the condition to return `true` if the value has leading and/or trailing whitespace and to return `false` if the value is _only_ whitespace.
--
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@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mtien-apache commented on a change in pull request #5559: NIFI-9336 - Show icon for property values with leading or trailing whitespace
Posted by GitBox <gi...@apache.org>.
mtien-apache commented on a change in pull request #5559:
URL: https://github.com/apache/nifi/pull/5559#discussion_r759771597
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/jquery/propertytable/jquery.propertytable.js
##########
@@ -1763,13 +1771,26 @@
var propertyHistory = history[property];
// format the tooltip
- var tooltip = nfCommon.formatPropertyTooltip(propertyDescriptor, propertyHistory);
+ var propertyTooltip = nfCommon.formatPropertyTooltip(propertyDescriptor, propertyHistory);
- if (nfCommon.isDefinedAndNotNull(tooltip)) {
+ if (nfCommon.isDefinedAndNotNull(propertyTooltip)) {
infoIcon.qtip($.extend({},
nfCommon.config.tooltipConfig,
{
- content: tooltip
+ content: propertyTooltip
+ }));
+ }
+ }
+
+ var whitespaceIcon = $(this).find('div.fa-info');
+ if (whitespaceIcon.length && !whitespaceIcon.data('qtip')) {
+ var whitespaceTooltip = nfCommon.formatWhitespaceTooltip();
+
+ if (nfCommon.isDefinedAndNotNull(whitespaceTooltip)) {
+ whitespaceIcon.qtip($.extend({},
Review comment:
@mcgilman good catch! I've updated the `clear` function. Can your review again? Thanks!
--
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@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mcgilman merged pull request #5559: NIFI-9336 - Show icon for property values with leading or trailing whitespace
Posted by GitBox <gi...@apache.org>.
mcgilman merged pull request #5559:
URL: https://github.com/apache/nifi/pull/5559
--
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@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org