Annotation viewer components #278

hudson-newey · 2025-01-22T04:17:10Z

Annotation viewer components

Changes

Adds oe-annotate, oe-annotation, and oe-tag components
Adds error/warning messages if the user inputs an incorrect value to an enum restricted attribute
Removes the (currently broken) "results" dropdown from the doc site examples (this was present because downloading results didn't exist)
Removes erroneous TypeScript use of as const on component event name declarations

Related Issues

Fixes: #5 "Annotation Viewer Component"
Fixes: #279 "Allow templating components in any order"

Bugs fixed during refactoring

Fixes: #255 needed to migrate axes to chromeProvider mixin
Fixes: #153 needed to migrate axes to chromeProvider mixin
Fixes: #282 needed for spectrogram-top attribute
Fixes: #78 needed to fix surface sizing
Fixes: #90 needed to fix zero width collapse
Fixes: #281 A bug I thought I created during the spectrogram chromeHost migration, but turns out it was a long standing bug that I never noticed until now
Fixes: #100 in hind sight, this was not needed. I originally thought it was needed because (long story short) reactivity was broken within the axes component. But I'm not going to revert a bug fix

Fixes: "Axes component used overflow rendering for labels"
Fixes: "Removing axis title / labels doesn't shrink chrome"
Fixes: "Box model for media-controls host is incorrect"
Fixes: "Standalone indicator component can't shrink if not inside container"
Fixes: "Bug in spectrogram.spec.ts" snapshot tests where the spectrogram would not obey the min-height
Fixes: "Fixes a bug where the enum attribute converter would throw an error for null (empty values e.g. <oe-annotate tag-style></oe-annotate>)"
Fixes: "Bug where the spectrogram host element would not respect the spectrograms min-height (this could cause the canvas to overflow the spectrogram host element)"

Tasks not resolved by PR

If we want to display annotations in the verification grid, we will need to spec out some new properties that will be required and add it to the default grid tile template

Visual changes

CSS parts warning on doc site "components" page

Final Checklist

github-actions · 2025-01-22T04:25:20Z

🚀 Deployed on https://67c939b3f2f644dbcd02dd35--oe-web-components.netlify.app

atruskie

Generally not keen on using the term heading for annotation labels. You may have used a h element, but they're not actually headings for paragraphs.

have we considered using lit context for the chrome advertisement?

there are a bunch of snapshots that are just black - no box rendered. double check validity?

So i'd really like to see chrome components not have to extend and implement interfaces.

dev/spectrogram-axes.html

dev/spectrogram-indicator.html

docs/annotations.md

src/components/annotate/annotate.ts

src/models/annotation.ts

...ion-overflowing-annotations-corner-21577-ld-position-correctly-in-the-top-right-corner-1.png

...ion-overflowing-annotations-partia-9b6b8-tly-if-only-the-high-frequency-has-overflowed-1.png

...ion-overflowing-annotations-fully--f24c8-f-it-has-fully-overflowed-the-negative-y-axis-1.png

...ion-overflowing-annotations-fully--8fb0b-f-it-has-fully-overflowed-the-negative-x-axis-1.png

atruskie · 2025-02-04T23:01:43Z

src/components/annotate/annotate.ts

@@ -191,8 +163,8 @@ export class AnnotateComponent extends ChromeProvider(LitElement) implements Wit
    // math so that we only have to do one calculation
    //
    // TODO: we might want to make this inclusive e.g. >=
-    const isTimeInView = model.startOffset < temporalDomain[1] && model.endOffset > temporalDomain[0];
-    const isFrequencyInView = model.lowFrequency < frequencyDomain[1] && model.highFrequency > frequencyDomain[0];
+    const isTimeInView = model.startOffset > temporalDomain[0] && model.endOffset >= temporalDomain[1];


you've mixed up the domain indices here

Thanks for catching that. Updated code is below

atruskie · 2025-02-04T23:05:02Z

src/components/annotate/css/style.css

      When the annotation headings are positioned at the top of the spectrogram
-      (tag-style="spectrogram-top"), if the label is too large to fit inside the
-      annotations width, it can cause the heading to overflow and not be visible.
+      if the label is too large to fit inside the annotations width, it can
+      cause the heading to overflow and not be visible.


even with the change i'm still confused by this comment.

do you mean:

a) in mode spectrogram-top (then why are we talking about that in .style-edge?)
or b) in mode edge, but when an annotation is rendered right at the top of the spectrogram

I'm talking about option b in your example

If you're still confused by it, I definitely need to change the wording of the comment

src/components/annotate/css/style.css

src/mixins/chrome/chromeHost/chromeHost.ts

atruskie · 2025-02-04T23:15:57Z

src/models/unitConverters.ts

@@ -115,6 +115,15 @@ export class UnitConverter {
    this.inverseLinearScale(this.frequencyDomain.value, this.frequencyRange.value, this.frequencyInterpolator.value),
  );

+  public annotationRect(): Rect<Pixel> {


is it worth reusing DOMRect?

I have a couple of reasons for not using DOMRect

We have have generic type hints (unfortunately not type checking) for a Rect<Pixel>

The DOMRect is defined externally by the tc39 group and the TypeScript team, meaning that if we update TypeScript, DOMRect might change. This would sometimes be a benefit, but in this case, the Rect is not strongly dependent on the shape of the DOMRect standard

DOMRect has some additional properties beyond x (left), y (top), width, and height (see image below)

atruskie

generally: i wonder if we should be namespacing our event names

you've got a few tests missing, but otherwise looks great

it looks like a lot of the snapshots changed....
why? - discussed offline. But update test container to a useful height, e.g. 512px.

src/components/annotate/annotate.ts

src/models/unitConverters.ts

Fixes: #5

This commit also improves the performance of the annotate component because the component no longer performance an update loop whenever the component resizes. spectrogram-top tag style positioning can be dynamically updated using signals from the unit converter.

Now if you change an oe-annotations attribute (e.g. start-time) it will update in the annotation viewer component

"In case of ~~fire~~ cyclone: 1. git add 2. git push" This commit improves JS annotation label positioning and fully removes non-functional anchor positioning.

hudson-newey added enhancement New feature or request ui labels Jan 22, 2025

hudson-newey self-assigned this Jan 22, 2025

hudson-newey force-pushed the annotation-viewer branch from 8bd32b8 to 491e712 Compare January 22, 2025 04:24

atruskie requested changes Feb 4, 2025

View reviewed changes

atruskie reviewed Feb 4, 2025

View reviewed changes

hudson-newey marked this pull request as ready for review February 6, 2025 04:40

hudson-newey changed the title ~~[DRAFT] Annotation viewer components~~ Annotation viewer components Feb 25, 2025

hudson-newey requested a review from atruskie February 25, 2025 04:52

atruskie approved these changes Feb 27, 2025

View reviewed changes

src/components/annotate/annotate.ts Outdated Show resolved Hide resolved

src/models/unitConverters.ts Outdated Show resolved Hide resolved

hudson-newey added 19 commits March 5, 2025 10:39

feat: Create annotation viewer component

1719412

Fixes: #5

Fix inconsistent casing of docs/ file names

66a7f89

Update spec

eb99ffb

Start creating annotation component

55e89d2

Add reactive properties to components

234fa3e

Add bounding box math

cd0ffd4

Pull tag from oe-annotation component attributes

4026786

Add support for oe-tag component

885cd77

Add oe-tag tests

0673d36

Backfill and partially complete anotation viewer tests

a02eb01

Fix inconsistent casing of docs/ file names

b975d62

Backfill overflow tests

2b2bf46

Document and create dev pages for edge cases

71d16ec

backfill missing tests for AnnotationComponent

c6db05b

Add axes component

c14305f

Add annotation culling

a45c7cc

Partial annotation heading anchor positioning

c6bceef

fix zero height spectrogram having bad host size

32d04ac

Staging

043a838

hudson-newey and others added 14 commits March 5, 2025 10:44

Move mixin types to types/ directory

f878e0b

Fix lit updates removing tag element reference

89907d2

Handle reassignment of spectrogram edge case

98f80e2

Axes self review

73eccad

Add dev tooling to test width reactivity

9904810

Start writing style helper tests

3734e12

Square spectrogram rendering snapshot tests

c29cb6a

Review feedback

2fc5997

Backfill spectrogram scaling tests

af02606

Backfill resize tests

2a27657

Add spectrogram chrome tests

4cd8b0e

ChromeHost, ChromeProvider and spectrogram tests

4faec05

Add build test dev pages

5ff4885

Fix static bundling errors

c984588

hudson-newey force-pushed the annotation-viewer branch from a05c7a0 to c984588 Compare March 5, 2025 00:48

hudson-newey added 15 commits March 5, 2025 11:07

Fix merge errors

7a1c9b4

Add style add tests

2675b41

Add merge style tests

fc7996b

Finish add and remove style tests

babf1b0

getting ready to migrate to adoptStyles

2d2b5ed

Migrate finalizeStyles static method to adoptStyles

05c9228

Readd methods for merge.spec

e83ace8

Fully working annotation tests

e142f28

Add annotation component event tests

75c4554

Fix oe-annotation-update event

2076396

Add annotaiton model reactivity to annotation viewer

ec286aa

Now if you change an oe-annotations attribute (e.g. start-time) it will update in the annotation viewer component

Finish annotate tests

d9ffad8

Start removing position anchor for annotations

3ce5435

Staging js annotation label positioning changes

df0917f

"In case of ~~fire~~ cyclone: 1. git add 2. git push" This commit improves JS annotation label positioning and fully removes non-functional anchor positioning.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotation viewer components #278

Annotation viewer components #278

hudson-newey commented Jan 22, 2025 •

edited

Loading

github-actions bot commented Jan 22, 2025 •

edited

Loading

atruskie left a comment

atruskie Feb 4, 2025

hudson-newey Feb 5, 2025

atruskie Feb 4, 2025

hudson-newey Feb 5, 2025 •

edited

Loading

atruskie Feb 4, 2025

hudson-newey Feb 5, 2025

atruskie left a comment

Annotation viewer components #278

Are you sure you want to change the base?

Annotation viewer components #278

Conversation

hudson-newey commented Jan 22, 2025 • edited Loading

Annotation viewer components

Changes

Related Issues

Bugs fixed during refactoring

Tasks not resolved by PR

Visual changes

Final Checklist

github-actions bot commented Jan 22, 2025 • edited Loading

atruskie left a comment

Choose a reason for hiding this comment

atruskie Feb 4, 2025

Choose a reason for hiding this comment

hudson-newey Feb 5, 2025

Choose a reason for hiding this comment

atruskie Feb 4, 2025

Choose a reason for hiding this comment

hudson-newey Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

atruskie Feb 4, 2025

Choose a reason for hiding this comment

hudson-newey Feb 5, 2025

Choose a reason for hiding this comment

atruskie left a comment

Choose a reason for hiding this comment

hudson-newey commented Jan 22, 2025 •

edited

Loading

github-actions bot commented Jan 22, 2025 •

edited

Loading

hudson-newey Feb 5, 2025 •

edited

Loading