Skip to content
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

fetch previews only once per page load #296

Merged
merged 2 commits into from
Nov 24, 2023
Merged

Conversation

e-five256
Copy link
Member

@e-five256 e-five256 commented Nov 24, 2023

this is a simple change where, after clicking the preview button once, further clicking will only hide and show the content, with no further AJAX calls to fetch the content again. If it fails, it will act as it did before showing a click to retry prompt (clicking preview will hide and show this prompt, but clicking on the prompt itself works to refetch the content)

@asdfzdfj since you touched the preview stuff recently, want to check with you that this makes sense

edit: tested this with asdfzdfj's example post of linked youtube with 2 image inlines and it appears to work, fetched each preview only once and correctly hid/made visible the previews after that

further clicking of preview only hides and shows content, does not reload
@e-five256 e-five256 added bug Something isn't working low priority does not impact production code and removed bug Something isn't working labels Nov 24, 2023
Copy link
Contributor

@asdfzdfj asdfzdfj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given a bit of test drive, seems to be working fine

come to think of it, maybe the wording of the retry link label should be adjusted: "Click here to retry." since hiding and reopening the preview now no longer refresh the preview content and only that link would trigger refresh

let failedHtml =
`<div class="preview">
<a class="retry-failed" href="#"
data-action="preview#retry"
data-preview-url-param="${event.params.url}"
data-preview-ratio-param="${event.params.ratio}">
Failed to load. Click to retry.
</a>
</div>`

@e-five256
Copy link
Member Author

It'd be nice if it could be a loc token but I didn't see a quick example of that via JS. maybe it needs to load a twig template instead?

@asdfzdfj
Copy link
Contributor

It'd be nice if it could be a loc token but I didn't see a quick example of that via JS. maybe it needs to load a twig template instead?

some ways I could think of:

  • using symfony ux translator to expose some translations to js and then do it in the controller
  • template translation in twig and pass it to preview controller by controller values, something like:
<div data-controller="preview"
     data-preview-retry-label-value="{{ 'preview_click_to_retry'|trans }}">...</div>
export default class extends Controller {
    static values = {
        retryLabel: String,
    };
    // ...
    async show(event) {
        // ...
        let failedHtml = `<div><a>${this.retryLabelValue}</a></div>`
        // ...
    }
}

@e-five256
Copy link
Member Author

e-five256 commented Nov 24, 2023

This seems to work except for inline embeds. I'm stuck on adding it to https://github.com/MbinOrg/mbin/blob/main/src/Markdown/CommonMark/EmbedElement.php#L15-L19

I have 'data-preview-retry-label-value' => $this->translator->trans('preview_click_to_retry'), but it doesn't seem to add any attributes, even tried just adding 'data-foo' => 'bar', and still nothing.

Then again, maybe it doesn't effect past content if that markdown is parsed only once... hmm no even posting new content doesn't add any attributes to that html 😕 it renders <span class="preview" data-controller="preview"> regardless of what I do. maybe I'm looking at the wrong file... but if that's the case I have no idea what is rendering it, I can't find any other locations of data-controller preview

@e-five256
Copy link
Member Author

Maybe I should just change the hardcoded wording here, leave it alone for now, and open a separate PR for translating it to take a look at?

@asdfzdfj
Copy link
Contributor

it should be that file (EmbedElement) that's used for render links, but a more thorough cache flush/reload might be needed
(I tend to use something like php bin/console cache:clear && sudo service php-fpm reload && ( . ./.env.local; REDISCLI_AUTH=$REDIS_PASSWORD redis-cli FLUSHDB ) to flush/reload cache when I'm tinkering)

using ux-translator for this is starting to look like a better option...

@e-five256
Copy link
Member Author

e-five256 commented Nov 24, 2023

Alright not saying I'm giving up but pulling in an experimental bundle for this also seems like it'd require a lot of testing, but it's possible we want to do that anyways, we probably have more hardcoded copy elsewhere

I just went with changing the hardcoded copy for now to keep the changes here simple

@asdfzdfj
Copy link
Contributor

Maybe I should just change the hardcoded wording here, leave it alone for now, and open a separate PR for translating it to take a look at?

yeah I think that'd be good enough for now (the changes I pulled from kbin side is also hardcoded too)

Alright not saying I'm giving up but pulling in an experimental bundle for this also seems like it'd require a lot of testing, but it's possible we want to do that anyways, we probably have more hardcoded copy elsewhere

I also found another bundle: https://github.com/willdurand/BazingaJsTranslationBundle if you want to play around

@e-five256 e-five256 merged commit 0b7c370 into main Nov 24, 2023
7 checks passed
@e-five256 e-five256 deleted the e5/preview-single-fetch branch November 24, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low priority does not impact production code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants