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.

Sign up for GitHub

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

Jump to bottom

IBX-9322: ezobjectrelationlist field allows selecting the same content multiple times #1409

Merged
merged 5 commits into from
Jan 29, 2025
Merged

IBX-9322: ezobjectrelationlist field allows selecting the same content multiple times #1409

merged 5 commits into from
Jan 29, 2025

Conversation

lucasOsti
Copy link
Contributor

@lucasOsti lucasOsti commented Dec 13, 2024 •
edited
Loading

🎫 Issue IBX-9322

Description:

Screenshot 2025-01-24 at 11 57 29

For QA:

Documentation:

@lucasOsti lucasOsti force-pushed the IBX-9322-ezobjectrelationlist-multiple-selection-same-content branch from 1c5593e to ce73425 Compare December 13, 2024 13:41
@lucasOsti lucasOsti added Bug Something isn't working Ready for review labels Dec 17, 2024
@katarzynazawada katarzynazawada mentioned this pull request Dec 23, 2024
Draft
katarzynazawada
katarzynazawada requested changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@katarzynazawada katarzynazawada left a comment

Choose a reason for hiding this comment

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

Content that had already been selected should be disabled or somehow highlighted during the selection process.
Current state:

Screen.Recording.2025-01-07.at.09.08.08.mov

@lucasOsti lucasOsti force-pushed the IBX-9322-ezobjectrelationlist-multiple-selection-same-content branch 3 times, most recently from 4ce602b to c4c5c15 Compare January 10, 2025 09:32
@lucasOsti lucasOsti requested review from dew326 and bozatko January 10, 2025 09:36
@lucasOsti lucasOsti force-pushed the IBX-9322-ezobjectrelationlist-multiple-selection-same-content branch from c4c5c15 to 7fdd261 Compare January 10, 2025 09:40
@katarzynazawada katarzynazawada force-pushed the IBX-9322-ezobjectrelationlist-multiple-selection-same-content branch from 7fdd261 to d740367 Compare January 16, 2025 11:40
katarzynazawada
katarzynazawada requested changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@katarzynazawada katarzynazawada left a comment

Choose a reason for hiding this comment

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

There is one inconsistency, selected item is disabled, X button in the list also but Deselect all buton is available. After using it all items are deselected, user can select new one but when UDW is closed pre-selected items are visible that can be confusing.
See please

Screen.Recording.2025-01-16.at.13.14.14.mov

@barbaragr
Copy link

There is one inconsistency, selected item is disabled, X button in the list also but Deselect all buton is available. After using it all items are deselected, user can select new one but when UDW is closed pre-selected items are visible that can be confusing. See please

Screen.Recording.2025-01-16.at.13.14.14.mov

I don't know if it isn't related to this issue https://issues.ibexa.co/browse/IBX-8853 somehow.

@lucasOsti lucasOsti force-pushed the IBX-9322-ezobjectrelationlist-multiple-selection-same-content branch from 655bcf1 to 0d27279 Compare January 24, 2025 10:36
bozatko
bozatko approved these changes Jan 24, 2025
View reviewed changes
...e/ui-dev/src/modules/universal-discovery/components/selected-locations/selected.locations.js Outdated
}
}, [selectedLocations, isInitLocationsDeselectionBlocked]);

if (!allowConfirmation || (onlyInitSelectedLocationsAreSelected && isInitLocationsDeselectionBlocked)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: what do you think about putting all these conditions in a variable with a proper name to improve readability?

@lucasOsti
Copy link
Contributor Author

There is one inconsistency, selected item is disabled, X button in the list also but Deselect all buton is available. After using it all items are deselected, user can select new one but when UDW is closed pre-selected items are visible that can be confusing. See please
Screen.Recording.2025-01-16.at.13.14.14.mov

I don't know if it isn't related to this issue https://issues.ibexa.co/browse/IBX-8853 somehow.

This should be fixed by #1404

@juskora juskora force-pushed the IBX-9322-ezobjectrelationlist-multiple-selection-same-content branch from da609f1 to 064c725 Compare January 28, 2025 10:22
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@juskora juskora mentioned this pull request Jan 29, 2025
Closed
@juskora
Copy link
Contributor

juskora commented Jan 29, 2025

QA Approved on Ibexa DXP Commerce 4.6-dev.

@tischsoic tischsoic merged commit 11d3fde into 4.6 Jan 29, 2025
29 checks passed
@tischsoic tischsoic deleted the IBX-9322-ezobjectrelationlist-multiple-selection-same-content branch January 29, 2025 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bozatko bozatko bozatko approved these changes

@juskora juskora juskora approved these changes

@dew326 dew326 dew326 approved these changes

@tischsoic tischsoic tischsoic approved these changes

@GrabowskiM GrabowskiM Awaiting requested review from GrabowskiM

@katarzynazawada katarzynazawada Awaiting requested review from katarzynazawada

Assignees

@mikadamczyk mikadamczyk

@ciastektk ciastektk

@dew326 dew326

@ViniTou ViniTou

Labels
Bug Something isn't working QA approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@lucasOsti @barbaragr @juskora @tischsoic @dew326 @katarzynazawada @bozatko @mikadamczyk @ciastektk @ViniTou