-
Notifications
You must be signed in to change notification settings - Fork 3k
feat: BROS-194: Custom tags #8108
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
Conversation
Region/Result's `unselectRegion()` and connected code. The whole `ImageRegion` is not in use.
- `forEach()` -> `for .. of` conversions. MST maps were not touched except for `keys()` access which is easier with `for .. of`. - Bunch of optional chaining added to simplify code - `Number.isNaN()` instead of `isNaN()`
We needed it only because of confusion between `text: string` for `Text` tag and `text: string[]` for `TextArea` tag. We don't need to omit other values.
✅ Deploy Preview for label-studio-docs-new-theme canceled.
|
✅ Deploy Preview for label-studio-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for heartex-docs canceled.
|
✅ Deploy Preview for label-studio-playground ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #8108 +/- ##
===========================================
+ Coverage 70.02% 70.45% +0.43%
===========================================
Files 684 720 +36
Lines 51061 52178 +1117
Branches 8628 8823 +195
===========================================
+ Hits 35753 36763 +1010
- Misses 15308 15412 +104
- Partials 0 3 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
/git merge
|
This will fix validation error on import
We have no usages of functional `altIcon`, this wrapper is excess
We need to check whether it can be converted to number, so we need `isNaN()`. https://stackoverflow.com/questions/33164725
b434bf0
to
ace272f
Compare
ace272f
to
b434bf0
Compare
The main usage of this component was in `_renderAll()` and we don't use it anymore. The only usage left is in old UI which will be removed soon, so we basically don't use `Segment` component.
moving to separate var for better performance Co-authored-by: Nick Skriabin <767890+nick-skriabin@users.noreply.github.com>
@@ -80,14 +81,21 @@ const hotkeys = Hotkey("Annotations", "Annotations"); | |||
* @param value {Object} object to fix | |||
* @returns {Object} new object without value fields | |||
*/ | |||
function omitValueFields(value) { | |||
const omitValueFields = ff.isActive(ff.FF_CUSTOM_TAGS) ? (value) => { | |||
// @todo describe that we only omit `text` from TextArea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this true? The original code looks like it is possibly omitting far more 🤯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we needed it only because of this confusion. I added some info into commit message (60ca64c), but will write a comment here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a point in the code where the serialized result is deserialized, and at this point all result data is duplicated both in value and at the top level. Because in RichText we decided to store the text in the serialized result, and both RichText region and TextArea use the "text"
field to store their data, when passing it to the top level, we have to filter the "text"
field to avoid type conflicts. After determining the type, we add this value "manually" using applyAdditionalDataFromResult
.
However, yes, this is the only detected case at the moment.
(I really hope I remember the reason of this function correctly)
/git merge
|
toggleCollapsed={toggleCollapsed} | ||
/> | ||
{item.hideable && ( | ||
<RegionControls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we really want to remove all controls under this condition. There's functionality like "Copy Region Link"
, and I don't yet understand whether it requires removal or will operate as intended if retained.
|
||
addCustomTag(tag: string, definition: CustomTag) { | ||
this.addTag(tag.toLowerCase(), definition.model, definition.view); | ||
this.addObjectType(definition.model); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we won't be able to add something different from the object tag to the Custom tags.
Is it expectable behaviour?
Moved all enumerations and types to consts and reused them by condition.
/git merge
|
Summary
Enable platform support for custom tags and add backend validation for Chat data. This PR provides backend validation and a feature-flag switch that allows downstream apps to register custom tags.
Key highlights in this PR
Allow to register custom tag
model
,view
, andregion
in one call.web/libs/editor/src/core/Registry.ts
(addCustomTag
hooks intoaddTag
,addObjectType
,addRegionType
).Include registered items in all required places for a new tag to work
omitValueFields
below), ensuring registered region types are detected correctly.Utilize
hideable
param (previously unused in some UI paths)region.hideable
/item.hideable
when rendering visibility controls in Outliner and Details panel.web/libs/editor/src/components/SidePanels/OutlinerPanel/OutlinerTree.tsx
,web/libs/editor/src/components/SidePanels/DetailsPanel/RegionItem.tsx
.Simplify
omitValueFields
to only address Text vs TextArea confusionvalue.text
when it's an array (TextArea case) underFF_CUSTOM_TAGS
, preventing conflicts that caused wrong region/classification detection.web/libs/editor/src/stores/Annotation/Annotation.js
(omitValueFields
).Other Changes
Backend
TaskValidator
data types to includeChat
so tasks with chat data validate correctly.Feature flags
FF_CUSTOM_TAGS
override in development/OSS builds to allow experimentation and downstream registration of custom tags. No LS UI exposure in this PR.Affected Files (high-level)
label_studio/tasks/validation.py
web/libs/core/src/lib/utils/feature-flags/ff.ts
web/libs/editor/src/core/Registry.ts
web/libs/editor/src/stores/Annotation/Annotation.js
web/libs/editor/src/components/SidePanels/OutlinerPanel/OutlinerTree.tsx
web/libs/editor/src/components/SidePanels/DetailsPanel/RegionItem.tsx
Feature Flags
FF_CUSTOM_TAGS
: enabled by default in OSS/dev via override.Why
hideable
, and deserialization safety; no new user-facing features in LS.Testing
Rollout / Risk
Docs