PlanQK Token field #41
Conversation
- Rename SolveRequest to SolveChoice to be in line with the type in SolverPicker - In ProgressHandler keep track of the underlying SolverChoice of the SolverPicker, then create a SolveRequest when it is time to make a request
|
| export interface Authentication { | ||
| /** | ||
| * The token used to authenticate the solver | ||
| */ | ||
| token: string; | ||
| } |
There was a problem hiding this comment.
Why use this object instead of a plain string?
There was a problem hiding this comment.
Because it might not be a token in the future but some other authentication method. I envisioned this to reflect the AuthenticationOptions type. When more boolean options like supportsToken are added, their result will be stored in the Authentication type.
| text: string; | ||
| } | ||
|
|
||
| const urlRegex = /(https?:\/\/\S+|www\.\S+|\b\w+@\w+\.\w+(?:\.\w+)?\b)/; |
There was a problem hiding this comment.
Did you come up with this regex yourself? If yes, please add a few test cases. If no, please add a source
There was a problem hiding this comment.
I don't know where this is from anymore. I could use another regex and link the source.
src/components/TextWithLink.tsx
Outdated
| const parts: string[] = props.text.split(urlRegex); | ||
|
|
||
| return ( | ||
| <Box> |
There was a problem hiding this comment.
Considering that the object is called TextWithLinks, maybe use <Text> here?
| return ( | ||
| <Container> | ||
| <Text>This solver requires authentication</Text> | ||
| {authenticationOptions.supportsToken ? ( |
There was a problem hiding this comment.
Why do we have a supportsToken field if tokens are currently the only supported way?
There was a problem hiding this comment.
See my answer for the Authentication type above. My reason is extensibility.
| onChange={(e) => { | ||
| let newSolverChoice: SolverChoice = { | ||
| ...solverChoice, | ||
| authentication: { | ||
| token: e.target.value, | ||
| }, | ||
| }; | ||
|
|
||
| setSolverChoice(newSolverChoice); | ||
| props.setSolverChoice?.(newSolverChoice); | ||
| }} |
There was a problem hiding this comment.
can you extract this to a function?
| {solveRequest.requestedSolverId == undefined ? ( | ||
| {selectedSolver?.authenticationOptions !== undefined && | ||
| selectedSolver?.authenticationOptions !== null | ||
| ? Authentication(selectedSolver.authenticationOptions) |
There was a problem hiding this comment.
I'm not sure what you mean
There was a problem hiding this comment.
Oh, you mean the short circuit way to have optional elements, gotcha. That's a neat trick.
| {solverChoice.requestedSolverId == undefined ? ( | ||
| <SettingsView | ||
| problemType={props.problemType} | ||
| settingChanged={(settings) => { | ||
| let newSolveRequest: SolverChoice = { | ||
| ...solveRequest, | ||
| let newSolverChoice: SolverChoice = { | ||
| ...solverChoice, | ||
| requestedMetaSolverSettings: settings, | ||
| }; | ||
|
|
||
| setSolveRequest(newSolveRequest); | ||
| props.setSolveRequest(newSolveRequest); | ||
| setSolverChoice(newSolverChoice); | ||
| props.setSolverChoice(newSolverChoice); | ||
| }} | ||
| /> | ||
| ) : null} |
There was a problem hiding this comment.
You can use the conditional and (&&) operator for conditional rendering without an else block (https://react.dev/learn/conditional-rendering#logical-and-operator-)
|
|
@schweikart I'm done with my pass, let me know what you think :) |
|
Same functionality was merged with #53 |
No description provided.