fundamental-ngx
fundamental-ngx copied to clipboard
fix(platform): fixed multi-combobox same name issue
Related Issue(s)
closes #8587 and #8600
Description
In multi-combobox when a list is having items with same name, there was an error adding those items. for example, if there are two items with name "Apple" but types different, user should be able to check both the apple. It is fixed now.
User have to provide a unique id in lookup key field from the datasource as shown in the picture which will be used to compare each values among items.
Datasource:
Screenshots
Before:
After:
Please check whether the PR fulfills the following requirements
During Implementation
- Visual Testing:
- [x] visual misalignments/updates
- [x] check Light/Dark/HCB/HCW themes
- [x] RTL/LTR - proper rendering and labeling
- [x] responsiveness(resize)
- [x] Content Density (Cozy/Compact/(Condensed))
- [x] States - hover/disabled/focused/active/on click/selected/selected hover/press state
- [x] Interaction/Animation - open/close, expand/collapse, add/remove, check/uncheck
- [x] Mouse vs. Keyboard support
- [x] Text Truncation
- API and functional correctness
- [x] check for console logs (warnings, errors)
- [x] API boundary values
- [x] different combinations of components - free style
- [x] change the API values during testing
- Documentation and Example validations
- [x] missing API documentation or it is not understandable
- [x] poor examples
- [x] Stackblitz works for all examples
- Accessibility testing
- Browser Testing - Edge, Safari, Chrome, Firefox
PR Quality
- [x] the commit message(s) follows the guideline: https://github.com/SAP/fundamental-ngx/blob/main/CONTRIBUTING.md
- [x] tests for the changes that have been done
- [x] all items on the PR Review Checklist are addressed : https://github.com/SAP/fundamental-ngx/wiki/PR-Review-Checklist
- [x] Run npm run build-pack-library and test in external application
- [x] update
README.md
- [x] Breaking Changes Wiki
Check the deploy log for errors here: https://app.netlify.com/sites/fundamental-ngx/deploys
Name | Link |
---|---|
Latest commit | 136a0e3475792ccc7daa2407f96f6a72f9d125d4 |
Latest deploy log | https://app.netlify.com/sites/fundamental-ngx/deploys/632ac1efc5fffa000887211a |
Visit the preview URL for this PR (updated for commit 18dfe1c):
https://fundamental-ngx-gh--pr8624-fix-8587-multi-combo-mm46ixpn.web.app
(expires Fri, 23 Sep 2022 15:53:49 GMT)
🔥 via Firebase Hosting GitHub Action 🌎
@fkolar I have made changes and added js docs.
If the developer wants to use objects, but does not provide a lookupKey, I think we should automatically generate unique ID's for each item. But I would also like to keep this current setup if the developer does want to provide a lookupKey
HI @mikerodonnell89 , I think, This would be challenging. there are some enterprise setup, when working with complex objects and you need ID, as 99% of the time you will be fetching data out of your REST API -> from DB. This implementation was always aimed to Application level > Enterprise
I am not sure how to generate ID and then manage equality between objects. Every-time you type in something it triggers REST API call using DataProvider to fetch new data, so the generated IDs stored within components (selections) would no longer be equal to the data coming from backend.
We try to come up with the most performant solution that could work for enterprise applications. We are talking about real objects that can contains 50 fields+ some additional nested structure, so trying to compare object by reference or deep comparison is not ideal, This is why there was this idea of LookupKey
since we started to design this component. The only thing we were not able to manage well this final implementation, how we worked with the objects.
We could throw warning message to developer if lookupKey is not provided when they use Complex objects as Item. In original implementation we introduced a problem, and this is more a fix, than feature.
Merged, as we had many discussion about this behavior and did not get any response for 6 days, so we went ahead and merged this. As this was blocking issue for our adoption teams where customers raised a few issues. .