| [19:00:07] | * cosine has quit (*.net *.split) |
| [19:02:32] | * cosine has joined #farmos |
| [09:44:47] | <mstenta[m]> | What do y'all think about this as a simple first step before 2.0.0? https://github.com/farmOS/farmOS/pull/619 |
| [11:39:07] | <mstenta[m]> | Also for review: https://www.drupal.org/project/farm/issues/3244733#comment-14822561 |
| [11:40:10] | <mstenta[m]> | Note: I just whipped that one up as a quick proof of concept to keep the conversation going - if others don't think it's the right approach we can discuss - just eager to close stuff :-) |
| [12:20:12] | <symbioquine[m]> | <mstenta[m]> "Also for review: https://www...." <- Looks reasonable to me - though I don't know much more about it than what we talked about on that call... |
| [12:20:23] | <symbioquine[m]> | Will use Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay; work without layout_builder installed? |
| [12:20:52] | <mstenta[m]> | oh i think so? |
| [12:20:55] | <mstenta[m]> | it's a good question |
| [12:21:13] | <mstenta[m]> | i'll see right now... i'll just uninstall layout_builder and make sure everything still works :-) |
| [12:21:21] | <symbioquine[m]> | Cool |
| [12:33:20] | <mstenta[m]> | > Will `use Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay;` work without layout_builder installed? |
| [12:33:20] | <mstenta[m]> | works fine! |
| [12:33:33] | <mstenta[m]> | i think that makes sense actually... PHP has no concept of "modules installed" |
| [12:33:41] | <mstenta[m]> | that code exists, so it's happy |
| [12:33:46] | <mstenta[m]> | it's just not going to be used |
| [12:34:56] | <paul121[m]> | <mstenta[m]> "Also for review: https://www...." <- how does this work with multiple view modes? I think it only alters the `default` right now, which is good. But it seems that these `asset_preprocess__full` hooks would be run for all view modes? |
| [12:35:35] | <symbioquine[m]> | mstenta[m]: And it's part of core right so it's always present? |
| [12:35:37] | <mstenta[m]> | paul121: oh hmm, maybe my assumptions were wrong - i thought `full` WAS the view mode (but you're right it's technically called `default`) |
| [12:35:50] | <mstenta[m]> | symbioquine: correct |
| [12:36:17] | <mstenta[m]> | paul121: the map_popup view mode is also enabled |
| [12:36:30] | <mstenta[m]> | always has been - and that doesn't get affected by the layout logic |
| [12:36:33] | <paul121[m]> | yeah, maybe there is something weird where full == default ? |
| [12:36:38] | <mstenta[m]> | yeaa... maybe? |
| [12:36:42] | <mstenta[m]> | it's a good question |
| [12:38:08] | <mstenta[m]> | https://drupal.stackexchange.com/questions/166118/which-is-the-differenc... |
| [12:39:01] | <mstenta[m]> | > So, when you are going to node/1, there is by default no display for `full`, so it falls back to default. Whereas `teaser` does have one, so it uses different things. However, you can also enable a form display for `full` and then it will no longer use `default`. |
| [12:39:23] | <mstenta[m]> | 🤔 |
| [12:41:43] | <mstenta[m]> | i don't see where you can "enable a form display for `full`" anywhere in the UI |
| [12:42:10] | <mstenta[m]> | second answer says: |
| [12:42:10] | <mstenta[m]> | > |
| [12:42:10] | <mstenta[m]> | There is no difference between default and full, they are essentially the same thing. "Full" is just a view mode and for some entities that is the "Default" view mode (not all though). |
| [12:42:36] | <paul121[m]> | > view_mode can basically be anything you want, you can even make up them on the fly. |
| [12:42:36] | <mstenta[m]> | s/// |
| [12:42:36] | <paul121[m]> | ?? |
| [12:43:09] | <paul121[m]> | Well, I think what you could do is create a new full view/form display |
| [12:43:20] | <paul121[m]> | just like we made the map_popup |
| [12:43:54] | <paul121[m]> | Drupal core must provide a standard teaser view display for nodes |
| [12:44:00] | <mstenta[m]> | ah ok |
| [12:44:05] | <mstenta[m]> | yea that sounds right |
| [12:45:57] | <mstenta[m]> | ah hmm... there is a difference between "view mode" and "view display" |
| [12:46:20] | <mstenta[m]> | default is the display |
| [12:46:24] | <mstenta[m]> | full is the view mode |
| [12:46:28] | <paul121[m]> | so right now we're technically altering the default and full view displays, if someone happened to make a full then it would be changed |
| [12:47:01] | <mstenta[m]> | (see comments on the first answer) |
| [12:47:31] | <paul121[m]> | yea, grokking that too.... I think "view mode" is just the name for the parameter passed to EntityViewBuilderInterface, nothing more |
| [12:48:46] | <paul121[m]> | https://www.drupal.org/docs/drupal-apis/entity-api/display-modes-view-mo... |
| [12:48:51] | <mstenta[m]> | so do you see any issue with this logic? |
| [12:49:55] | <paul121[m]> | paul121[m]: wrong! view mode is the config entity too: `core.entity_view_mode.node.full.yml` |
| [12:50:10] | <mstenta[m]> | ah |
| [12:50:21] | <paul121[m]> | but also the parameter passed to that method. which, a view_mode config doesn't exists, it uses default :-) |
| [12:53:23] | <mstenta[m]> | so just to recap: |
| [12:53:23] | <mstenta[m]> | 1. if you go to "Manage display" for an asset type, and you enable "Use layout builder", it creates a `core.entity_view_display.asset.BUNDLE.default` config |
| [12:53:23] | <mstenta[m]> | 2. we are implementing `hook_preprocess_HOOK()` to alter the theming of `asset__full`, `log__full`, and `plan__full` |
| [12:53:49] | <paul121[m]> | mstenta[m]: I don't think the `default` view mode should be hard-coded here: https://git.drupalcode.org/project/farm/-/merge_requests/75/diffs#a35cfa... |
| [12:53:49] | <paul121[m]> | only because this *could be* the `full` view mode too. kinda an edge case |
| [12:53:49] | <mstenta[m]> | so, we are specifically altering the full view mode... which is using the default display |
| [12:54:41] | <mstenta[m]> | what should be there then? full? I'm not sure that would work |
| [12:55:10] | <mstenta[m]> | default is the config that gets written to when you enable layout builder |
| [12:55:12] | <paul121[m]> | yeah me neither. I wonder if we should use a different preprocess hook somehow |
| [12:55:30] | * spitz234[m] has quit (Ping timeout: 252 seconds) |
| [12:56:24] | <paul121[m]> | well you could write config to the full display as well (if you created that view mode) |
| [12:56:46] | * spitz234[m] has joined #farmos |
| [12:56:56] | <paul121[m]> | but that is an existing issue, not new. this change just revealed it to me :-) |
| [12:57:11] | <mstenta[m]> | yea that makes sense |
| [12:57:31] | <mstenta[m]> | well i don't want to open up a too much of a can of worms |
| [12:58:31] | <mstenta[m]> | still trying to understand the potential issue... |
| [12:58:49] | <mstenta[m]> | right now, someone will most likely enable layout builder on the default display |
| [12:59:08] | <mstenta[m]> | but you're saying that IF they created a full view mode, and then enabled it on that, then this would break... ? |
| [12:59:24] | <mstenta[m]> | i think i see what you mean - does seem very edgy |
| [12:59:56] | <mstenta[m]> | hmm |
| [13:00:17] | <paul121[m]> | yes. but full display mode is a common convention for drupal nodes it seems, so not too far out |
| [13:01:28] | <paul121[m]> | common convention... it's like baked into the core code as the default view mode haha :P |
| [13:01:33] | <mstenta[m]> | yea |
| [13:07:08] | <paul121[m]> | maybe hook_entity_display_build_alter: https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Entity!entity.api... |
| [13:07:25] | <paul121[m]> | but we prob don't need to fix this right away |
| [13:14:44] | <paul121[m]> | an alternate approach... we could depend on layout builder and dynamically build/override the default view display to use the two column layout as needed? its a bit more complicated so I understand why we didn't do that in the beginning. but if might solve all the problems now? |
| [13:58:02] | <mstenta[m]> | yep! |
| [13:58:13] | <mstenta[m]> | we could consider that! |
| [13:58:50] | <mstenta[m]> | in which case i would say let's punt until after 2.0.0 |
| [13:59:03] | <mstenta[m]> | so: should we postpone this issue? |
| [13:59:21] | <paul121[m]> | yea, that sounds good |
| [14:00:15] | <mstenta[m]> | alrighty! |
| [14:02:48] | <mstenta[m]> | this one good to merge? https://github.com/farmOS/farmOS/pull/619 |
| [14:03:09] | <mstenta[m]> | (seems like you suggested as much paul121 - just wanted to be sure before i pull the trigger) |
| [14:03:13] | <paul121[m]> | yep! |
| [14:03:17] | <paul121[m]> | LGTM |
| [14:03:22] | <mstenta[m]> | cool! |
| [14:03:34] | <mstenta[m]> | i will focus on the timestamp thing now |
| [14:03:44] | <mstenta[m]> | just needs tests |