Attempt at externalizing layout from extensions #829
Conversation
ext/setup/theme.php
Outdated
| public function format_option(string $name, $html, ?string $label, bool $table_row, bool $label_row = false): string | ||
| { | ||
| $output = ""; | ||
| if ($table_row) { | ||
| $output .= "<tr><th colspan='".($label_row ? 2 : 1)."' style='text-align: ".($label_row ? 'center' : 'right')."'>"; | ||
| } | ||
| if (!is_null($label)) { | ||
| $output .= "<label for='{$name}'>{$label}</label>"; | ||
| } | ||
|
|
||
| if ($table_row) { | ||
| $output .= "</th>"; | ||
| } | ||
|
|
||
| if ($table_row && $label_row) { | ||
| $output .= "</tr><tr>"; | ||
| } | ||
|
|
||
| if ($table_row) { | ||
| $output .= "<td colspan='".($label_row ? 2 : 1)."'>"; | ||
| } | ||
| $output .= $html; | ||
| if ($table_row) { | ||
| $output .= "</td>"; | ||
| } | ||
| if ($table_row) { | ||
| $output .= "</tr>"; | ||
| } | ||
|
|
||
| return $output; | ||
| } |
There was a problem hiding this comment.
main.php still uses a lot of small non-overridable functions to generate tables (I think we should avoid that) but at least this one can be overridden now.
EDIT: Thankfully, those functions are rarely used outside of the setup extension itself.
start_table and end_table are the only ones that appear in multiple extensions, but even then they're mostly used once before the options and once after. We can remove most of them safely.
There was a problem hiding this comment.
FWIW the current approach of adding lots of TD/TH/TR elements seems flexible but messy - ideally this'd be another place where the extension just returns a list of {setting_name, human_name, value, display_style} and the theme takes care of HTML, but I'm not sure how to do that without massively reducing flexibility...
(This isn't something that needs fixing right away, just something that I have in the back of my mind)
There was a problem hiding this comment.
It seems flexible right until you don't want to use tables 😄
Also, I'm noticing extensions create their tables in very different ways, which explains why I had so much trouble getting everything to look consistent on my theme. There's even setupblocks that don't use tables at all and just populate the items in the div.
I know it's going to look a lot less flexible when I commit this. But giving extensions that much flexibility leads to inconsistencies...
I'm also trying to send everything through this format_option function, which I now renamed to format_item and takes care of labels and text as well, and I created other functions in theme.php to wrap them in the table elements (the ones one would override to get rid of tables entirely)
We can discard it if you think it's too much anyway. I'm adapting all extensions to the revised format now, I'll probably commit after that.
ext/view/theme.php
Outdated
| @@ -85,7 +85,17 @@ protected function build_info(Image $image, $editor_parts): string | |||
| <table style='width: 500px; max-width: 100%;' class='image_info form'> | |||
| "; | |||
| foreach ($editor_parts as $part) { | |||
| $html .= $part; | |||
| // This could be using something like MicroHTML? but the contents need to allow HTML anyway. | |||
There was a problem hiding this comment.
You can add unsafe generic HTML into the middle of an otherwise-safe structure with rawHTML like
use function MicroHTML\rawHTML;
[...]
TH(rawHTML($part->header))
This also makes it easy to search for rawHTML, so if we see some invalid HTML being generated somewhere, we can focus on those parts :)
There was a problem hiding this comment.
That is nice. Will add this to my TODO list :)
ext/tag_edit/theme.php
Outdated
| <td width='80px' rowspan='4'>$h_av</td> | ||
| </tr> | ||
| "; | ||
| "); //TODO: Implement removed <td width='80px' rowspan='4'>$h_av</td> |
There was a problem hiding this comment.
This is the only thing stopping me from merging at the moment, as avatars are a pretty important part of user-identity ^^
I'd be tempted to implement it by adding it to ext/view/theme.php - it's not ideal, but it would be a small step back while everything else here is several steps forwards
There was a problem hiding this comment.
oh, don't worry. I'll implement it accordingly. This PR is not meant to be merged yet as it is a bit less polished, because I'm working on multiple things at the same time. I wanted to make sure I wasn't making a big mistake and have to discard too much work. I'm working on setup's layout at the moment, which was a big pain when I tried to theme it.
| @@ -141,7 +141,7 @@ public function onPageRequest(PageRequestEvent $event) | |||
| return; | |||
| } | |||
|
|
|||
| $uobe = new UserOptionsBuildingEvent($display_user, new SetupPanel($user_config)); | |||
| $uobe = new UserOptionsBuildingEvent($display_user, new SetupPanel($user_config, new SetupTheme)); | |||
There was a problem hiding this comment.
This is the only thing that's not working with the SetupBlock changes. How should I access SetupTheme? Maybe UserConfigTheme should extend SetupTheme now? I feel like it should.
EDIT: I know it's a lot 😆 but should be very customizable in themes? There's a few table styling things needed, and a few css helper classes to be applied. But the layout is there.
ext/setup/theme.php
Outdated
| if ($label_is_header) { | ||
| return $label . $this->build_setup_row($html); | ||
| } | ||
|
|
||
| return $this->build_setup_row($label . $html); |
There was a problem hiding this comment.
Found a bug here. I'll work on it tomorrow.
It's pretty harmless tho, creates an empty table row when formatting a full width label.
Decided to go for a different branch for this, so we can easily discard it or merge onto the other one, since I'm still trying to find the workflow we should go for with this rework.
If we want to let themes completely replace layouts, like replacing this table with a flexbox or grid, we should externalize the TR, TH, TD wrappers found in extensions. This is just one example of them.
I decided to go for a custom class for parts of this
ImageInfoblock but this is not the only place where an object withheader,bodyandorderis needed, so I can see Parts becoming its own thing like Blocks but with just the bare minimum information. They would be useful for events that build blocks like this one. Maybe we end up using an array and not a dedicated class for this, idk.The part building process is not definitive, it's just a quick prototype.
I wanted to use MicroHTML on
ext/view/theme.php'sbuild_infobut the contents of the table still needed to allow for HTML to be injected. This may be improved as well.There's a few TODOs here, mainly notes for myself. Also, sorry @shish, I know this is a lot of work and a lot to think about and review. I'm trying to maximize my contributions since this is my last free week before starting uni.