You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@felix.apache.org by "sagarmiglani (via GitHub)" <gi...@apache.org> on 2023/06/21 08:26:03 UTC
[GitHub] [felix-dev] sagarmiglani opened a new pull request, #212: FELIX-6614 WebConsole configMgr saves an empty value in list properites
sagarmiglani opened a new pull request, #212:
URL: https://github.com/apache/felix-dev/pull/212
This pull request addresses the issue of an empty value being saved when a list property has no value (FELIX-6614). The proposed solution is to filter out the request query parameters that have empty values.
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] sagarmiglani commented on pull request #212: FELIX-6614 WebConsole configMgr saves an empty value in list properites
Posted by "sagarmiglani (via GitHub)" <gi...@apache.org>.
sagarmiglani commented on PR #212:
URL: https://github.com/apache/felix-dev/pull/212#issuecomment-1633810655
> The new "add" button is now displayed for every list property. Shouldn't it only be displayed if there is no value yet?
@cziegeler Thank you for reviewing, I made this decision based on our current implementation, which includes a '+' button next to each field. This button enables users to add a new empty field below it, granting them the ability to add values at any index, excluding the first one. By keeping the '+' button visible at all times, we provide users with the option to add new values at the first index as well. However, if you strongly recommend against displaying the '+' button at all times, I am open to making changes accordingly.
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] kwin commented on pull request #212: FELIX-6614 WebConsole configMgr saves an empty value in list properites
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on PR #212:
URL: https://github.com/apache/felix-dev/pull/212#issuecomment-1637624924
@sagarmiglani Thanks for the clarification, makes sense to me. But as I am not a committer you would need @cziegeler's approval and merge.
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] cziegeler commented on pull request #212: FELIX-6614 WebConsole configMgr saves an empty value in list properites
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on PR #212:
URL: https://github.com/apache/felix-dev/pull/212#issuecomment-1623035482
The new "add" button is now displayed for every list property. Shouldn't it only be displayed if there is no value yet?
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] kwin commented on pull request #212: FELIX-6614 WebConsole configMgr saves an empty value in list properites
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on PR #212:
URL: https://github.com/apache/felix-dev/pull/212#issuecomment-1633837561
I think this comment referred only to the topmost add icon (not bound to an item, compare with the screenshot).
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] cziegeler merged pull request #212: FELIX-6614 WebConsole configMgr saves an empty value in list properites
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler merged PR #212:
URL: https://github.com/apache/felix-dev/pull/212
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] kwin commented on a diff in pull request #212: FELIX-6614 WebConsole configMgr saves an empty value in list properites
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #212:
URL: https://github.com/apache/felix-dev/pull/212#discussion_r1253967553
##########
webconsole/src/main/resources/res/ui/config.js:
##########
@@ -348,6 +349,25 @@ function printConfigurationInfo( /* Element */ parent, obj )
var spanCounter = 0;
+/* Element */ function createAddButton(prop) {
+ spanCounter++;
+ var newId = prop + spanCounter;
+
+ var addButton = createElement("input", null,
Review Comment:
maybe one could reuse the same code here and in createSpan(...) to reduce duplication.
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] kwin commented on a diff in pull request #212: FELIX-6614 WebConsole configMgr saves an empty value in list properites
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #212:
URL: https://github.com/apache/felix-dev/pull/212#discussion_r1236748770
##########
webconsole/src/main/resources/res/ui/config.js:
##########
@@ -674,7 +690,7 @@ $(document).ready(function() {
$.ajax({
type : 'POST',
url : pluginRoot + '/' + $(this).attr('__pid'),
- data : $(this).find('form').serialize(),
+ data : filterEmptySerializedKeyValues($(this).find('form').serialize()),
Review Comment:
Also currently the UI allows to remove the last item from a collection without any means to add one item afterwards.
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] cziegeler commented on pull request #212: FELIX-6614 WebConsole configMgr saves an empty value in list properites
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on PR #212:
URL: https://github.com/apache/felix-dev/pull/212#issuecomment-1623091773
Let's not put too many changes into this single issue. The most important part is to avoid saving the empty value. So I suggest, we keep the add/delete buttons after each field. And if there is no value yet, only display the add button
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] sagarmiglani commented on a diff in pull request #212: FELIX-6614 WebConsole configMgr saves an empty value in list properites
Posted by "sagarmiglani (via GitHub)" <gi...@apache.org>.
sagarmiglani commented on code in PR #212:
URL: https://github.com/apache/felix-dev/pull/212#discussion_r1238096004
##########
webconsole/src/main/resources/res/ui/config.js:
##########
@@ -674,7 +690,7 @@ $(document).ready(function() {
$.ajax({
type : 'POST',
url : pluginRoot + '/' + $(this).attr('__pid'),
- data : $(this).find('form').serialize(),
+ data : filterEmptySerializedKeyValues($(this).find('form').serialize()),
Review Comment:
Thanks @kwin, Quickly updated the changes according to your suggestions, please let me know your thoughts :)
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] kwin commented on a diff in pull request #212: FELIX-6614 WebConsole configMgr saves an empty value in list properites
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #212:
URL: https://github.com/apache/felix-dev/pull/212#discussion_r1236747502
##########
webconsole/src/main/resources/res/ui/config.js:
##########
@@ -674,7 +690,7 @@ $(document).ready(function() {
$.ajax({
type : 'POST',
url : pluginRoot + '/' + $(this).attr('__pid'),
- data : $(this).find('form').serialize(),
+ data : filterEmptySerializedKeyValues($(this).find('form').serialize()),
Review Comment:
How to support collections with empty values then? I think this requires more changes, i.e. always expose the + button but no empty input field if the collection is empty.
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] kwin commented on pull request #212: FELIX-6614 WebConsole configMgr saves an empty value in list properites
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on PR #212:
URL: https://github.com/apache/felix-dev/pull/212#issuecomment-1623038983
I think the best UX would be
- one delete button per item
- one add button per list
- possibility to reorder (maybe leveraging https://jqueryui.com/draggable/#sortable)
The "+" per item is very uncommon and is IMHO not needed if there is a possibility to reorder.
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] kwin commented on pull request #212: FELIX-6614 WebConsole configMgr saves an empty value in list properites
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on PR #212:
URL: https://github.com/apache/felix-dev/pull/212#issuecomment-1623035491
@cziegeler Can you have a look?
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] sagarmiglani commented on pull request #212: FELIX-6614 WebConsole configMgr saves an empty value in list properites
Posted by "sagarmiglani (via GitHub)" <gi...@apache.org>.
sagarmiglani commented on PR #212:
URL: https://github.com/apache/felix-dev/pull/212#issuecomment-1637585391
@kwin I am also referring to the topmost add icon.
Here is the comparison:
|Scenario|Present scenario|Proposed solution|
|---|---|---|
|1|**When there is not value in array, an empty field is present along with "+" and "-" button**|**When there is no value in array, only "+" button is visible (which is visible all the time)**|
|2|User can add a new row by clicking on "+" button and remove the row by clicking on "-"|User can add a new row by clicking on "+" button and remove the row by clicking on "-"|
|3|If there are multiple rows, user can add a row in-between by clicking on "+" button. i.e if there are rows 1,2,3; user can add a new row between 2 and 3 by clicking "+" button of row 2|If there are multiple rows, user can add a row in-between by clicking on "+" button. i.e if there are rows 1,2,3; user can add a new row between 2 and 3 by clicking "+" button of row 2|
|4|**User can not add any row at first index, while can add at any other index.**|**User can add any row at first index by clicking on topmost "+" button**|
|5|**If user deletes all rows, there is no option to add a new row**|**If user deletes all rows, a new row can still be added by clicking on topmost "+" button**|
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] cziegeler commented on pull request #212: FELIX-6614 WebConsole configMgr saves an empty value in list properites
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on PR #212:
URL: https://github.com/apache/felix-dev/pull/212#issuecomment-1638029753
@sagarmiglani Thanks for the comparison. I think the item where we have different opinions is row 4 - while I agree that it would be consistent to allow to add a value to the first row, I think this is a limited use case - in many cases the ordering of the values does not matter. But with the button always there, it takes more screen space than today.
But I'm fine either way. if you think we should go with what you propose, I'm happy to apply the change
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org