Skip to content
This repository has been archived by the owner on Apr 18, 2024. It is now read-only.

fix: LSDV-1476: Fix selected=true for Taxonomy #1239

Merged
merged 24 commits into from
Oct 4, 2023

Conversation

hlomzik
Copy link
Collaborator

@hlomzik hlomzik commented Mar 14, 2023

Now <Choice selected="true"> will work inside Taxonomy as well.

PR fulfills these requirements

  • Commit message(s) and PR title follows the format [fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made ex. fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
  • Tests for the changes have been added/updated (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • Self-reviewed and ran all changes on a local instance (for bug fixes/features)

Change has impacts in these area(s)

(check all that apply)

  • Product design
  • Frontend

Describe the reason for change

Choice tag has selected bool attribute to define preselected options in config. But this didn't work for Choices inside Taxonomy.

What does this fix?

Adds preselectedValues to Taxonomy, fix init of Choices inside Shared Store in order for them to have proper values.

What is the new behavior?

"Usual" value is preselected in new annotations in configs like this:

<Taxonomy name="items" toName="text">
  <Choice value="Usual" selected="true" />
  ...
</Taxonomy>

What is the current behavior?

Choices in Taxonomy can't be preselected.

What libraries were added/updated?

none

Does this change affect performance?

updateValue() is called for every Choice in Taxonomy at initialization, so it may affect performance, but slightly.

Does this change affect security?

nope

What alternative approaches were there?

preselectedValues are current standard for such things, so no alternatives here.
For Choice value simpler approach can be used to get value/alias directly, not via getter, but it requires duplication of value retrieval logic (array/string).

What feature flags were used to cover this change?

ff_dev_2100_preselected_choices_250422_short to enable preselected choices
fflag_fix_front_dev_3617_taxonomy_memory_leaks_fix made it harder

Does this PR introduce a breaking change?

(check only one)

  • Yes, and covered entirely by feature flag(s)
  • Yes, and covered partially by feature flag(s)
  • No
  • Not sure (briefly explain the situation below)

What level of testing was included in the change?

(check all that apply)

  • e2e
  • integration
  • unit

Which logical domain(s) does this change affect?

Taxonomy, Dynamic Choices

First try, not fully working.
Now `<Choice selected="true">` will work inside Taxonomy as well.
@hlomzik hlomzik marked this pull request as ready for review June 23, 2023 15:27
@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2023

Codecov Report

Patch coverage: 70.27% and project coverage change: +0.04 🎉

Comparison is base (18fa5c2) 64.97% compared to head (c592549) 65.02%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1239      +/-   ##
==========================================
+ Coverage   64.97%   65.02%   +0.04%     
==========================================
  Files         439      439              
  Lines       27464    27490      +26     
  Branches     7176     7182       +6     
==========================================
+ Hits        17846    17874      +28     
+ Misses       9618     9616       -2     
Impacted Files Coverage Δ
src/env/development.js 0.00% <0.00%> (ø)
src/env/production.js 86.84% <ø> (+2.22%) ⬆️
src/stores/AppStore.js 56.04% <ø> (+0.37%) ⬆️
src/utils/feature-flags.ts 100.00% <ø> (ø)
src/tags/control/Taxonomy/Taxonomy.js 88.35% <52.63%> (-5.40%) ⬇️
src/tags/control/Choices.js 74.39% <90.90%> (+2.55%) ⬆️
src/mixins/DynamicChildrenMixin.js 69.04% <100.00%> (ø)
src/stores/Annotation/Annotation.js 72.30% <100.00%> (+0.04%) ⬆️
src/tags/control/Choice.js 91.30% <100.00%> (ø)
src/tags/object/Paragraphs/HtxParagraphs.js 75.89% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Not the best way to do it anyway
@hlomzik
Copy link
Collaborator Author

hlomzik commented Jul 7, 2023

/git merge master

Successfully pushed new changes:
Merge remote-tracking branch 'origin/master' into fb-lsdv-1476/preselected-taxonomy (b5b0d22)

Workflow run

@hlomzik
Copy link
Collaborator Author

hlomzik commented Sep 28, 2023

/git merge master

Already up-to-date. Nothing to commit.

Workflow run

Children should be init before other processes like setDefaultValues()
This case can happen during store init in Label Stream,
task will be loaded later.
@hlomzik hlomzik merged commit c71c229 into master Oct 4, 2023
14 checks passed
@hlomzik hlomzik deleted the fb-lsdv-1476/preselected-taxonomy branch October 4, 2023 13:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants