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

SITES-26750 Accordion : Content editor dialog is adding "None" twice in Expanded Items field #2890

Merged
merged 9 commits into from
Nov 12, 2024

Conversation

RaduADumitru
Copy link
Contributor

@RaduADumitru RaduADumitru commented Nov 7, 2024

Q                       A
Fixed Issues? Fixes #1, Fixes #2
Patch: Bug Fix? Yes
Minor: New Feature? No
Major: Breaking Change? No
Tests Added + Pass? No
Documentation Provided Yes (code comments and or markdown)
Any Dependency Changes? No
License Apache License, Version 2.0

Steps to reproduce bug:

  1. Add an accordion component and include three items, such as text components.
  2. Navigate to the properties tab and select the "Single item expansion" checkbox.
  3. Open the dropdown menu, which displays all three items along with an extra "None" option.
  4. Go to the Items tab and delete one item, for example, item 2.
  5. Return to the properties tab and open the dropdown menu again, observing that there are now two "None" options.

This is caused by the way expanded select is updated: it removes all the current items where value is not "", then adds "None", then readds items from children editor. Issue is fixed by also cleaning up "None" value, whereas this didn't happen before. Adapts fix from #2857

Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.15%. Comparing base (081c26d) to head (7ba51e7).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2890   +/-   ##
=========================================
  Coverage     87.15%   87.15%           
  Complexity     2692     2692           
=========================================
  Files           235      235           
  Lines          7188     7188           
  Branches       1100     1100           
=========================================
  Hits           6265     6265           
  Misses          365      365           
  Partials        558      558           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RaduADumitru RaduADumitru requested a review from a team November 7, 2024 09:36
@RaduADumitru RaduADumitru changed the title SITES-26750 Accordion component is showing "None" in properties tab. SITES-26750 Accordion : Content editor dialog is adding "None" twice in Expanded Items field Nov 7, 2024
@RaduADumitru
Copy link
Contributor Author

RaduADumitru commented Nov 11, 2024

2 more possible issues I noticed with the original fix, upon further inspection:

  • The check for removing "None" option doesn't account for localization: if the localized value of "None" is different than the original, then the "None" option won't be removed. Could be fixed by checking for the localized value in the check instead
  • The check, since it uses includes() instead of strict equality, can also accept values that simply contain "None" in the text but are not this exact value. Could be fixed by using strict equality instead of includes

Will rectify

@RaduADumitru
Copy link
Contributor Author

RaduADumitru commented Nov 11, 2024

Changed approach to do full cleanup before reinitializing expanded select, including of "None" element. Couldn't see any elements of value "" which we do want to keep, other than "None" element. Would be IMO the safest way to avoid other unforeseen problems, such as if other elements with value "" are added.

@RaduADumitru RaduADumitru merged commit 77dd95c into main Nov 12, 2024
15 checks passed
@RaduADumitru RaduADumitru deleted the SITES-26750 branch November 12, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants