-
Notifications
You must be signed in to change notification settings - Fork 636
Apps for custom components concepts #1386
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
base: main
Are you sure you want to change the base?
Conversation
| const HOLD_DURATION = 2000; // 2 seconds | ||
| const COOLDOWN_DURATION = 1500; // cooldown after trigger | ||
| const CIRCUMFERENCE = 2 * Math.PI * 45; // circle circumference |
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.
suggestion: Since these are unchanging consts, it is preferred to define them outside of this function as module level consts. eg:
const HOLD_DURATION = 2000; // 2 seconds
const COOLDOWN_DURATION = 1500; // cooldown after trigger
const CIRCUMFERENCE = 2 * Math.PI * 45; // circle circumference
export default function() {
}| return () => { | ||
| if (animationFrame) cancelAnimationFrame(animationFrame); | ||
| }; |
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.
suggestion: All of the events that are created must be cleaned up in here too. So that would be all of the mousedown, mouseup, etc events.
| @@ -0,0 +1,89 @@ | |||
| export default function ({ | |||
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.
question: Do we think this example is worthwhile? I think it wouldn't be ideal if people are building forms on their own in custom components given the great form + input support that already exists in Streamlit. What what specific value do we think this example illustrates for users for custom components?
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.
It's the custom frontend validation logic that I though people might want. The danger button kind of is a simplified, one-dimensional variation of "frontend validation" so I'm not 100% invested in this particular example if it's not to your tastes.
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.
Yeah, I think the danger button is probably enough to cover that possibility. With that in mind, I'd prefer to omit this one just so that it doesn't seem like people should reinvent the wheel here. Might also give LLMs the wrong idea!
| function polarToCartesian(angle) { | ||
| const rad = ((angle - 90) * Math.PI) / 180; | ||
| return { | ||
| x: cx + radius * Math.cos(rad), | ||
| y: cy + radius * Math.sin(rad), | ||
| }; | ||
| } | ||
|
|
||
| function describeArc(start, end) { | ||
| const startPoint = polarToCartesian(start); | ||
| const endPoint = polarToCartesian(end); | ||
| const largeArc = end - start > 180 ? 1 : 0; | ||
| return `M ${startPoint.x} ${startPoint.y} A ${radius} ${radius} 0 ${largeArc} 1 ${endPoint.x} ${endPoint.y}`; | ||
| } |
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.
suggestion: For any functions and/or consts that aren't part of the component lifecycle, it would be better to define those outside of the export default function for readability. It helps separate the business logic from the code necessary to wire something up to the custom component.
📚 Context
This PR adds app source code for embedded apps in the custom components v2 concept docs.
This also carves out an exception in the pre-commit hook so that all example code in the python example source directories use a tab of four spaces. (Code is copied between the app source and docs when writing, and linting is used to make sure it's all well-formatted. I want all the code that we show in the docs to have a consistent indent across languages--four spaces--and it's easier to avoid errors when the docs and the source are exact copies.)
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.