Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FELIX-6614 WebConsole configMgr saves an empty value in list properites #212

Merged
merged 2 commits into from Jul 20, 2023

Conversation

sagarmiglani
Copy link
Contributor

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.

@sagarmiglani sagarmiglani requested a review from kwin June 22, 2023 07:12
spanCounter++;
var newId = prop + spanCounter;

var addButton = createElement("input", null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe one could reuse the same code here and in createSpan(...) to reduce duplication.

@cziegeler
Copy link
Contributor

The new "add" button is now displayed for every list property. Shouldn't it only be displayed if there is no value yet?
new

@kwin
Copy link
Member

kwin commented Jul 6, 2023

I think the best UX would be

The "+" per item is very uncommon and is IMHO not needed if there is a possibility to reorder.

@cziegeler
Copy link
Contributor

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

@sagarmiglani
Copy link
Contributor Author

sagarmiglani commented Jul 13, 2023

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 "at top" 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 top '+' button at all times, I am open to making changes accordingly.

@kwin
Copy link
Member

kwin commented Jul 13, 2023

I think this comment referred only to the topmost add icon (not bound to an item, compare with the screenshot).

@sagarmiglani
Copy link
Contributor Author

@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

@kwin
Copy link
Member

kwin commented Jul 17, 2023

@sagarmiglani Thanks for the clarification, makes sense to me. But as I am not a committer you would need @cziegeler's approval and merge.

@cziegeler
Copy link
Contributor

@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

@cziegeler cziegeler merged commit 9819a6e into apache:master Jul 20, 2023
@sagarmiglani sagarmiglani deleted the FELIX-6614 branch July 20, 2023 04:56
cziegeler pushed a commit that referenced this pull request Aug 28, 2023
…es (#212)

* FELIX-6614 WebConsole configMgr saves an empty value in list properites

* FELIX-6614 WebConsole configMgr saves an empty value in list properites

---------

Co-authored-by: Sagar Miglani <saga@adobe.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants