-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[UI Framework] Define consistent default, small, and wide sizing for form fields, and support them within ExpressionItems. #12190
[UI Framework] Define consistent default, small, and wide sizing for form fields, and support them within ExpressionItems. #12190
Conversation
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.
@@ -80,3 +80,6 @@ $toolBarItsemSpacing: 10px; | |||
$globalFormControlHorizontalPadding: 12px; | |||
$globalFormControlPadding: 3px $globalFormControlHorizontalPadding 4px; | |||
$globalFormInputHeight: 30px; | |||
$globalFormFieldDefaultWidth: 180px; | |||
$globalFormFieldSmallWidth: 60px; | |||
$globalFormFieldLargeWidth: 400px; |
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.
Might want to follow a pattern with your sizes.
$globalFormFieldSmallWidth: $globalFormFieldDefaultWidth / 3;
$globalFormFieldLargeWidth: $globalFormFieldDefaultWidth * 2;
...etc. Just some sort of math so that when they sit next to each other they look like they were planned.
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 like the idea of applying some rhythms that make sense, but as it is they're just chosen because they look good in different scenarios. Can we get together at some point and figure out a rhythm that we can apply here so the values aren't so arbitrary?
That bug you found with the Watcher table is actually a problem with their markup. I'll address that in another issue. |
@snide I looked through through the UI and didn't find anything weird. Can you take another look and approve the PR? |
…d support them within ExpressionItems.
35b0236
to
737e456
Compare
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 - This fixes the issues we were seeing in the Watcher UI. I defer to @snide to scrutinize the quality of the less. :)
@BigFunger This should fix the problem you've been having with putting different sizes of inputs into an expression.