Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 26, 2024
Merged

Conversation

BrunoQuaresma
Copy link
Collaborator

Before:
Screenshot 2024-01-24 at 13 11 31

After:
Screenshot 2024-01-24 at 13 11 11

@BrunoQuaresma BrunoQuaresma requested a review from a team January 24, 2024 16:13
@BrunoQuaresma BrunoQuaresma self-assigned this Jan 24, 2024
@BrunoQuaresma BrunoQuaresma requested review from Kira-Pilot and removed request for a team January 24, 2024 16:13
@BrunoQuaresma BrunoQuaresma changed the title refactor(site): improve parameters fields refactor(site): improve parameter fields Jan 24, 2024
@BrunoQuaresma BrunoQuaresma changed the title refactor(site): improve parameter fields refactor(site): improve parameters field Jan 24, 2024
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) => {
Copy link
Member

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which function? The map?

Copy link
Member

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

Copy link
Collaborator Author

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?

Copy link
Member

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!

Copy link
Collaborator Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, good point

Copy link
Member

@Kira-Pilot Kira-Pilot left a 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.

Copy link
Collaborator Author

@BrunoQuaresma BrunoQuaresma left a 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

image.png

@BrunoQuaresma BrunoQuaresma merged commit 0ba035a into main Jan 26, 2024
@BrunoQuaresma BrunoQuaresma deleted the bq/simplify-parameters branch January 26, 2024 14:31
@github-actions github-actions bot locked and limited conversation to collaborators Jan 26, 2024
@BrunoQuaresma
Copy link
Collaborator Author

Related to #11742

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants