-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fix margins inside dataTables insight controls #23733
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: 5.x-dev
Are you sure you want to change the base?
Conversation
| .widget & { | ||
| padding: 12px 12px 0; | ||
| } |
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.
ℹ️ Add margins when the insight table is inside a widget
| {% if period == 'day' %} | ||
| <div class="row"> | ||
| <div class="col s12 m6 l4 input-field"> | ||
| <div class="col s12 m12 l12 input-field"> |
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.
ℹ️ No need to keep a free space, so this input will take all the available space.
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 this makes it less usable, can we keep the previous width on larger screens?
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.
You’re right. We could do better. I’ll add a max-width to fix this 👍
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.
Better like this?
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.
Apologies, it still doesn't sit 100% with me. Not sure we should be relying on content width as the minimal width and adding max width to things that have column layout applied instead of relying on the grid behaviour.
I'll ask for other opinion here since I don't want to hold it up. @sgiehl what's your view here? If you're happy with the change, feel free to approve.
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 first issue you raised was that the form was unusable when it was too wide.
We cannot solve this problem with the grid system, because the page could take up the entire width of the screen. Two-thirds of an infinite element could still be too wide to be usable.
That's why I think a “max-width” property seems complementary to the grid system in this case.
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.
Same here, I think we should keep UI controls consistent, not always grow in the remaining space. In wider views, it's basically 3 columns layout with 2 columns used. We can go to 2 columns when the content of the dropdowns would get cut off.
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.
Since the columns are set in HTML, we cannot really do what we want here.
In the previous screenshot, it was already too large to be comfortable in large screens, so I changed to propose something that I found a bit better.
What do you think?
c7f9028 to
e6d974d
Compare
|
|
||
| <div class="row"> | ||
| <div class="col s12 m4 l4 input-field"> | ||
| <div class="col s12 m6 l6 input-field"> |
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.
ℹ️ Took all available space too.
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.
Apologies, it still doesn't sit 100% with me. Not sure we should be relying on content width as the minimal width and adding max width to things that have column layout applied instead of relying on the grid behaviour.
I'll ask for other opinion here since I don't want to hold it up. @sgiehl what's your view here? If you're happy with the change, feel free to approve.
Co-authored-by: Michal Kleiner <[email protected]>
Description:
Fix margins of controls for insight data table in widgets:
Review