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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Shortcodes gallery (non-unique id, multiple loading <script> tag) #1126

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

v20100v
Copy link
Contributor

@v20100v v20100v commented Dec 18, 2023

Related to bug issue #1125

@v20100v
Copy link
Contributor Author

v20100v commented Dec 18, 2023

Some proposals in this merge request

Changes in ./shortcodes/gallery.html

  1. Proposition for a UUID more robust (prevent same id in case of galleries placed in timeline item)
    {{ $random := delimit (shuffle (seq 1 9)) "" }}
    {{ $id := delimit (slice "gallery" $random now.UnixNano) "-" }}
    
  2. Remove the tag <script>, because it was included multiples times when use multiples shortcode gallery in markdown. We need to include once. To do it, we create a specific js (./gallery.js) included in ./layouts/partials/vendor.html

Changes in package.json

Other proposals (which have nothing to do with the shortcode gallery)

  1. When you first deploy this project in a dev environment, it's strange that you have already installed the node package Rimraf as global dependencies (otherwise you get an error: package not installed). This package is install as a local node development dependencies. Remove preinstall ?

  2. The custom node script fullinstall": "npm run preinstall && npm install && npm run postinstall" is not necessary. When you use npm install, the preinstall and postinstall node scripts are run automatically by npm before and after the package is installed, respectively.

  3. Add scripts (dev-windows & build-windows) compatibles on Windows system (see cross-platform env variables in Node.js)

@v20100v v20100v changed the title 馃悰 : Shortcodes gallery (non-unique id, multiple loading <script> tag) 馃悰 Shortcodes gallery (non-unique id, multiple loading <script> tag) Dec 19, 2023
@nunocoracao nunocoracao merged commit d17678b into nunocoracao:dev Jan 4, 2024
2 checks passed
@MaikelChan
Copy link
Contributor

MaikelChan commented Jan 12, 2024

Turns out that this PR has brought back the issue of overlapping images in the gallery.

Back when I did the PR fixing the overlapping, $(window).on("load", function () { /* code */ }); was the only way I found to reliably fix that. Otherwise the event might be called before all images finished loading, causing packery not setting the layout of the gallery correctly.

@v20100v
Copy link
Contributor Author

v20100v commented Jan 15, 2024

Hello @MaikelChan,

I'm looking in detail, and trying to find a solution on this overlapping image problem.You have undoubtedly found the right clue, on the problem of loading the complete DOM with the Packery library.

@nisetynet
Copy link

blowfish/assets/js/shortcodes/gallery.js

@v20100v
Hello.
Thank you for developing the blowfish theme.

Based on MaikelChan's observations about the problem of overlapping images, what about the following code?
I have tested it dozens of times, and the problem does not occur.
At least in my environment.

function _getDefaultPackeryOptions() {
    return {
        percentPosition: true,
        gutter: 5,
        resize: true
    };
}

(function init() {
    console.debug("Set on load callback.");

    $(window).on("load", function () {
        console.groupCollapsed('[DEBUG] Gallery feature enable');
        let packeries = [];
        let nodeGalleries = document.querySelectorAll('.gallery');

        nodeGalleries.forEach(nodeGallery => {
            // TODO : implement a reader of Packery configuration _getPackeryOptions; for example by reading data-attribute
            let packery = new Packery(nodeGallery, _getDefaultPackeryOptions());
            packeries.push(packery);
        });

        console.log("Galleries founded and initialized with packery", packeries);
        console.groupEnd();
    });
})();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants