- Notifications
You must be signed in to change notification settings - Fork298
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
Entity layout and minor UI changes#832
base:3.x
Are you sure you want to change the base?
Conversation
Some before/after examples... Assets
Logs
|
| /* Hide layout regions if there is no content. */ | ||
| .layout--twocol .layout__region:not(:has(div)){ | ||
| display: none; | ||
| } |
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.
Hmm finding some situation where this might not work.
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.
This seemed to be caused by an empty<div> that seems like it would have displayed error/warning messages but it didn't render any and I haven't been able to reproduce. I was in-between switching branches and clearing cache so it's possible it was a fluke but I do wonder if this is an issue.
I will try and get some more screenshots (mobile!) with more data on these pages. Most notably log quantities and files that are attached to the page. |
A couple more screenshots with more data... definitely a few things to clean up, but the mobile LGTM! Anyone have thoughts on the below?@mstenta@Farmer-Eds-Shed@Fosten@symbioquine
Asset
Log
Log mobile
|
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.
definitely a few things to clean up, but the mobile LGTM!
Just revisiting this, on mobile the Log type + Status should probably at the top of the screen. As is, these are in the "right" column which gets moved down below the "left" column. Not sure the best solution... either move Log type + Status, or move the right column above the left on smaller screens? I think this will be better with#847
| 'weight' => [ | ||
| 'view' => 95, | ||
| 'view_display_options' => [ | ||
| 'label' => 'hidden', | ||
| 'type' => 'farm_map_geofield', | ||
| 'settings' => [ | ||
| 'output_format' => 'wkt', | ||
| 'output_escape' => TRUE, | ||
| ], | ||
| 'weight' => 95, |
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.
Instead of explicitly settingview_display_options here (which requires setting all the other bits and sub-settings), can we leave this as it was, but put a line after the call to\Drupal::service('farm_field.factory')->baseFieldDefinition($options); which simply sets thelabel tohidden?
For example:
$fields['geometry'] = \Drupal::service('farm_field.factory')->baseFieldDefinition($options);// Hide the geometry field label in the view display.$view_display_options = $fields['geometry']->getDisplayOptions('view');$view_display_options['label'] = 'hidden';$fields['geometry']->setDisplayOptions('view', $view_display_options);This makes it very explicit what we're doing, and means that if any of those other settings change infarm.field_factory in the future we don't need to remember to update them here as well.
| 'view_display_options' => [ | ||
| 'label' => 'hidden', | ||
| 'type' => 'farm_map_geofield', | ||
| 'settings' => [ | ||
| 'output_format' => 'wkt', | ||
| 'output_escape' => TRUE, | ||
| ], | ||
| 'weight' => 95, | ||
| ], |
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 as above)
It seems like there are still some bigger questions to answer / decisions to make with this PR generally (although I love where we're headed with it)! I'm inclined to wait until after the 3.3.0 release so we have more time to work through it. |
We decided on the dev call today to merge#847 first for farmOS v3.3.0, and then rebase this and pick up where we left off with it. |
My original motivation for this was to improve the display of the "Add new comment" field on entities. Ultimately I think the best solution is to make each of our layout regions use the "gin-layer-wrapper" styles so they get a background color/shadow and are lifted on the page to match the existing comment styles. Then decreasing the size of the comment text headers (h2 is pretty big) makes things look nice.
Last, some minor changes to make other css in farm_ui_theme consistent.