-
-
Notifications
You must be signed in to change notification settings - Fork 43
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 QA page analysis chart #1725
Conversation
Updated to move thresholds to tooltip per @emma-sg and @Shrinks99 feedback: ![]() |
Happy with this at this point! |
Nice work! |
Co-authored-by: Tessa Walsh <tessa@bitarchivist.net>
() => | ||
html`<sl-tooltip | ||
content=${qaRun.state.startsWith("stop") | ||
? msg("This analysis run was stopped and is not complete.") |
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.
Nitpick, but this would include stopping
, which isn't yet stopped. Maybe we should explicitly list stopped states in utils/crawler
, by activeCrawlStates
, to keep it all in one place?
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.
Oh, I suppose that's true, but don't we only show finished qa runs here to select from? https://github.com/webrecorder/browsertrix/blob/main/frontend/src/pages/org/archived-item-detail/ui/qa.ts#L482-L484
import { pluralize } from "./localization"; | ||
|
||
// Add to this as necessary! | ||
const plurals = { |
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.
Nice, thanks for this!
return stats; | ||
}, | ||
args: () => | ||
[this.orgId!, this.crawlId!, this.qaRunId, this.authState] as const, |
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.
This is pretty awesome
Resolves #1589
Changes
Note, I deviated from the wireframes to remove the "QA Analysis" heading since we only have one card now. It also feels like "Page Match Analysis" is a more precise heading for this particular chart. Open to alternatives.
Manual testing
Screenshots
Follow-ups
We'll probably want to refactor
btrix-meter
after conversation toTailwindComponent
to simplify reusability.