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

Add Annotation Search and Verification Pages #2145

Merged
merged 106 commits into from
Oct 30, 2024
Merged

Conversation

hudson-newey
Copy link
Member

@hudson-newey hudson-newey commented Oct 4, 2024

Verification Interface

This pull request creates an annotations search page & accompanying verification grid to the workbench client.

Changes

Features

  • Add open-ecoacoustics web components as a dependency
  • Add annotations search page
  • Add verification interface page
  • Add @angular/elements allowing us to create custom elements using Angular components (was needed for full Lit + Angular interoperability within slotted content)
  • Add semantic selectors to wip component
  • Add disabled attributes to date-time-input.component start/end date/time inputs
  • Add pageSize attribute to pagination template
  • You can now add response headers to the SSR server using the angular.json serve.options.headers object

Bug Fixes

  • Fix the previously non-functional UnsavedInputGuard
  • Fix docker image publish workflow doubling the inputted tag name on manual dispatches
  • Fix docker version build numbers now use an incrementing number instead of the number of commits in the tag
  • Fixes a bug with route titles when viewing profiles
  • Fixes a bug where typeahead input component pills would not be vertically centered
  • Fix failing CI due to deprecated dependencies

Code Quality

  • Disable AOT during development builds and disable lib check (don't typecheck third party library code). These changes result in decreased dev build time by ~40-50%
  • Add new test helpers selectFromTypeahead and getElementByInnerText

Issues

Fixes: #2140

Visual Changes

image

Annotations search page


image

Verification grid page


image

Verification grid with context card expanded

Final Checklist

  • Assign reviewers if you have permission
  • Assign labels if you have permission
  • Link issues related to PR
  • Ensure project linter is not producing any warnings (npm run lint)
  • Ensure build is passing on all browsers (npm run test:all)
  • Ensure CI build is passing
  • Ensure docker container is passing (docs)

@hudson-newey hudson-newey force-pushed the verification-interface2 branch from 8b99162 to c07e905 Compare October 28, 2024 04:28
Copy link
Contributor

github-actions bot commented Oct 28, 2024

Unit Test Results

         4 files           4 suites   6m 55s ⏱️
15 988 tests 15 580 ✔️ 408 💤 0
16 136 runs  15 728 ✔️ 408 💤 0

Results for commit 78ba8fe.

♻️ This comment has been updated with latest results.

Comment on lines +213 to +219
if (this.routeSiteId) {
siteIds = [this.routeSiteId];
} else if (this.routeRegionId) {
siteIds = Array.from(this.routeRegionModel.siteIds);
} else {
siteIds = Array.from(this.routeProjectModel.siteIds);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be better if these actually just filtered by the entities that are restricted rather than by site ids.

  • it avoids the additional request
  • and it's more semantically correct.

audio recording supports filtering by site, region, and project

Copy link
Member Author

@hudson-newey hudson-newey Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Audio recordings support filtering by associated site, region, and project models.

Because this data model constructs filter conditions for the audio_events API, our filter condition would have to look something like

// POST /audio_events/filter
{
	"filters": {
		// notice the double "." on associated models
		"audio_recordings.projects.id": {
			"in": [1, 2, 3, 4]
		}
	}
}

However because this is a filter condition on an association of an association it seems to fail

image


Potentially resolved by

QutEcoacoustics/baw-server#689

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file a separate issue and cross link

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 123 to 131
// because the verification grid keybindings are scoped at the component
// level, we automatically focus the verification grid component so that
// users don't have to manually focus the verification grid to start using
// shortcuts
//
// without this automatic focusing, the user would have to click on the
// verification grid (e.g. to make a sub-selection) before being able to
// use the shortcut keys
this.verificationGridElement.nativeElement.focus();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if we need a focus prompt/indicator on the verification grid... it could be confusing if keyboard shortcuts suddenly don't work

Copy link
Member Author

@hudson-newey hudson-newey Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

I've added a warning indicator if the verification grid loses focus (bound to the blur event)

I originally tried some text "Shortcuts are disabled" with a tooltip and button to re-gain focus, but there was a lot of cumulative shift, and the elements would move around a lot, so I ended up going with a simple warning icon + tooltip

src/app/helpers/context/context-decorators.ts Outdated Show resolved Hide resolved
src/app/models/AudioRecording.ts Outdated Show resolved Hide resolved
src/app/services/models/annotation.service.ts Show resolved Hide resolved
@hudson-newey hudson-newey force-pushed the verification-interface2 branch from ec35a25 to 78ba8fe Compare October 30, 2024 04:45
@hudson-newey hudson-newey merged commit 03fa8fe into master Oct 30, 2024
21 checks passed
@hudson-newey hudson-newey changed the title Verification Interface Add Annotation Search and Verification Pages Oct 30, 2024
@hudson-newey hudson-newey deleted the verification-interface2 branch October 30, 2024 05:20
@hudson-newey hudson-newey mentioned this pull request Dec 6, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create verification Interface
2 participants