fundamental-ngx icon indicating copy to clipboard operation
fundamental-ngx copied to clipboard

fix(platform): fixed multi-combobox same name issue

Open skaquib123 opened this issue 2 years ago • 4 comments

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. image

Datasource: image

Screenshots

Before:

image

After:

2022-08-30_10-33-07 (1)

Please check whether the PR fulfills the following requirements

During Implementation
  1. 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
  1. 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
  1. Documentation and Example validations
  • [x] missing API documentation or it is not understandable
  • [x] poor examples
  • [x] Stackblitz works for all examples
  1. Accessibility testing
  2. 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

skaquib123 avatar Aug 30 '22 05:08 skaquib123

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

netlify[bot] avatar Aug 30 '22 05:08 netlify[bot]

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 🌎

github-actions[bot] avatar Aug 30 '22 05:08 github-actions[bot]

@fkolar I have made changes and added js docs.

skaquib123 avatar Sep 20 '22 15:09 skaquib123

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.

fkolar avatar Sep 21 '22 06:09 fkolar

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. .

fkolar avatar Sep 27 '22 11:09 fkolar