-
Notifications
You must be signed in to change notification settings - Fork 39
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
Use Prior Knowledge in annotation #51
Conversation
@@ -3,7 +3,7 @@ import OpenAI from "openai"; | |||
import { useAppState } from "../state/store"; | |||
import { availableActions, availableActionsVision } from "./availableActions"; | |||
import { ParsedResponseSuccess } from "./parseResponse"; | |||
import { fetchKnowledge } from "./knowledge/fetchKnowledge"; | |||
import { type Knowledge } from "./knowledge"; |
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.
[just curious] why do we need to include 'type' here? is it a convention that as long as it's a type definition, the keyword 'type' should be included when importing?
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.
yes, it's better to clearly indicate type importing to differentiate from value importing
let prompt = | ||
formatPrompt(taskInstructions, previousActions) + | ||
`Current page progress: ${viewportPercentage.toFixed(1)}%`; | ||
if (knowledge.length > 0) { | ||
if (knowledge.notes != null && knowledge.notes?.length > 0) { |
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.
can be simplified as if (knowledge.notes && knowledge.notes?.length > 0) {
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 think it's good to do != null
for non-boolean values
@@ -134,6 +113,37 @@ function isTouchedElement(elem: Element) { | |||
); | |||
} | |||
|
|||
function getAriaLabel(elem: Element): string { | |||
// aria-labelledby has higher priority than aria-label |
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.
do we need to check both "aria-labelledby" and "aria-label"? Or these two always exist at the same time?
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 check aria-labelledby first, if it doesn't exist, check aria-label. that's the standard way
No description provided.