- Notifications
You must be signed in to change notification settings - Fork924
refactor(site): improve parameters field#11802
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
| This decision was made because rich parameter inputs are more visually dense than standard text fields. | ||
| */} | ||
| <div css={{ display: "flex", flexDirection: "column", gap: 36 }}> | ||
| {parameters.map((parameter, index) => { |
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.
NIT: I might lift this function outside of the JSX.
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.
Which function? The map?
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.
the callback that the map takes
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 is quite normal in react codebases but any specific concern?
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 feel like it lowers general readability because its a longer callback that defines multiple variables, but no strong concerns!
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 see! But in this case, we would have to pass 3+ arguments to the function so I'm not sure if extracting it to a new function or component would improve that much but I can understand the reasoning.
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.
good point, good point
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! I'll defer to your design expertise - I'm assuming we feel that the orange "immutable" label is still a strong enough signal to communicate that a new workspace must be made if an owner ever wants to change those values.
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.
Design is updated to
Related to#11742 |
Before:
After:
