-
Notifications
You must be signed in to change notification settings - Fork 170
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 Token Counter Displayer in Repl #2775
Add Token Counter Displayer in Repl #2775
Conversation
b5944f4
to
32d9e1d
Compare
Pull Request Test Coverage Report for Build 7889640712Details
💛 - Coveralls |
Corresponding backend PR: source-academy/backend#1062 |
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.
Thanks for working on this, looking forward to the new additions 👍 just some comments below:
type: 'notification' | ||
}); | ||
} | ||
const customNotification = state[workspaceLocation].customNotification; |
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 for future plans, or already used?
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 most likely for future plans. Not yet used.
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.
Would it make more sense for customNotification
to be customNotifications
(a string[]
) instead then? Since it's reasonable to assume you can have either 0, 1, or more than 1; there's no reason to only have either 0 or 1 notification present.
Anyway, it's not used now, so I'll just approve the PR, but perhaps you can create an issue: either remove it (cause it's unused), or make it support multiple notifications?
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.
LGTM, thanks for the quick fixes! Please refer to the thread above regarding custom notifications.
Thanks again!
Add Token Counter Displayer in Repl
Description:
This pull request creates a new assessment config which enables a token counter. This token counter is a notification which lets the user know how many tokens their program has. This notification can be seen as an output in the Repl once the program is run.
Tasks:
Notes:
Note that modifying custom Repl notifications hasn't been implemented yet (possible future updates).
Co-authored-by: Lau Xiang Yu