open-y-subprojects / openy_map

Map feature from Open Y DIstribution
GNU General Public License v3.0
1 stars 14 forks source link

Use renderPlain to ensure lazy_builder placeholders are rendered too. #61

Closed Sardis93 closed 1 year ago

Sardis93 commented 1 year ago

After update of ycloudyusa/yusaopeny package to 10.2.14.1 the location pins no longer have today's hours displayed: image This is because the today's hours are rendered using lazy_builder, similarly to the https://git.drupalcode.org/project/lb_branch_hours_blocks/-/blob/1.1.x/src/Plugin/Block/BranchHoursBlock.php?ref_type=heads#L184 and the markup looks like this:

<div class="today-hours position-relative">
    <div class="information">
      <span class="todays-hours-text pr-1">
        <i class="fas fa-clock pr-2 mr-1"></i><span class="d-inline d-md-none">Today</span><span class="d-none d-md-inline">Today's hours</span>:
      </span>
      <drupal-render-placeholder callback="ymca_branch.hours_today:generateHoursToday" arguments="0=%5B%7B%22Monday%22%3A%225%3A00am%20-%208%3A00pm%22%2C%22Tuesday%22%3A%225%3A00am%20-%208%3A00pm%22%2C%22Wednesday%22%3A%225%3A00am%20-%208%3A00pm%22%2C%22Thursday%22%3A%225%3A00am%20-%208%3A00pm%22%2C%22Friday%22%3A%225%3A00am%20-%208%3A00pm%22%2C%22Saturday%22%3A%228%3A00am%20-%201%3A00pm%22%2C%22Sunday%22%3A%221%3A00pm%20-%205%3A00pm%22%7D%2C%5B%7B%22holiday%22%3A%22Easter%20Sunday%22%2C%22hours%22%3A%22Closed%22%2C%22date_value%22%3A%222023-04-09%22%7D%2C%7B%22holiday%22%3A%22Memorial%20Day%22%2C%22hours%22%3A%228%3A00am-4%3A00pm%22%2C%22date_value%22%3A%222023-05-29%22%7D%2C%7B%22holiday%22%3A%22Independence%20Day%22%2C%22hours%22%3A%228%3A00am-4%3A00pm%22%2C%22date_value%22%3A%222023-07-04%22%7D%2C%7B%22holiday%22%3A%22Labor%20Day%22%2C%22hours%22%3A%228%3A00am-4%3A00pm%22%2C%22date_value%22%3A%222023-09-04%22%7D%2C%7B%22holiday%22%3A%22Thanksgiving%20Day%22%2C%22hours%22%3A%22Closed%22%2C%22date_value%22%3A%222023-11-23%22%7D%2C%7B%22holiday%22%3A%22Christmas%20Eve%22%2C%22hours%22%3A%22Closed%22%2C%22date_value%22%3A%222023-12-24%22%7D%2C%7B%22holiday%22%3A%22Christmas%20Day%22%2C%22hours%22%3A%22Closed%22%2C%22date_value%22%3A%222023-12-25%22%7D%2C%7B%22holiday%22%3A%22New%20Year%5Cu0027s%20Eve%22%2C%22hours%22%3A%221%3A00pm-4%3A00pm%22%2C%22date_value%22%3A%222023-12-31%22%7D%2C%7B%22holiday%22%3A%22New%20Year%5Cu0027s%20Day%22%2C%22hours%22%3A%225%3A00am-4%3A00pm%22%2C%22date_value%22%3A%222024-01-01%22%7D%5D%5D" token="nCcMX1VLACV5nw7k8JsJcBTJ-nZnvV9EgMEKbcln1g0"></drupal-render-placeholder>
    </div>
  </div>

image

From what i can gather, the placeholder is not rendered because we use $this->renderer->render(): https://github.com/open-y-subprojects/openy_map/blob/main/src/OpenyMapDataWrapper.php#L61

Earlier, it was renderRoot(): https://github.com/open-y-subprojects/openy_map/commit/952882f819bf788f675e8c43a735dbc1b17c91df But it caused another problem: https://github.com/open-y-subprojects/openy_map/issues/35 So i propose to use renderPlain(), this way the placeholders are rendered and no bubbling of attachments happens.

podarok commented 1 year ago

Makes sense, thank you @Sardis93

andriokha commented 1 year ago

Hi, I was directed to this as I had some (very!) peripheral involvement in the earlier issue (#35) but was on vacation. My understanding is that Renderer::render() is not directly the problem: if we take the resulting markup and display it on the page, the placeholders will be replaced. I think the underlying problem is that we don't directly display the result on page, we pass it as an attached JS setting, where placeholders won't get replaced. I think one solution would be to render the pins to a hidden area and let the JS clone them from there.

I think a potential problem with the current solution using renderPlain() is that we discard the attachments (css, js, cacheability metadata) of the rendered node.

Sardis93 commented 1 year ago

Hi, @andriokha !

I believe we don't need the attachments anyway, since we only need the actual data coming from teaser view mode. Any and all styling/js should be attached through the openy_map theme function. What do you think?

andriokha commented 1 year ago

Hi @Sardis93!

Any and all styling/js should be attached through the openy_map theme function

I don't think that's an assumption the distro can really make. I think it's valid for a developer to override a branch teaser template and use attach_library but IIUC currently that won't work. And there's also cacheability metadata, which can be a real pain to debug. It's true that a common usecase is to have a teaser listing beneath the map, in which case the listing view solves most (all?) issues described, but if I use the map alone and update the template to use a custom library, I'd expect that to fail.