Problem/Motivation

I'm still looking into what exactly is going on, but I have thumbnail nav for a main slider and everything is working except the previous arrow.

I changed the splide/base library to point to the un-minified code and started commenting things out to see if it was a core Splide bug or a library bug, and I tracked it down to this line in splide.base.js in the instance.on('arrows:mounted') handler:

if (o.type === 'slide') {
  go(prev, 0, end);
  go(next, end, 0);
}

If I comment that out everything works just fine. I'll keep looking but just in case anyone knows what that is actually supposed to be doing and why removing it fixes the arrows it could save me some time.

CommentFileSizeAuthor
#24 Screenshot_2024-03-30-00-32-49-004_com.opera_.mini_.native.jpg332.21 KBgausarts
#10 Screenshot_2024-03-29-02-25-38-234_com.opera_.mini_.native.jpg329.24 KBgausarts

Issue fork splide-3436319

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

KarlShea created an issue. See original summary.

ravi kant’s picture

ravi kant commented

I am also getting same issue.
I try to figure out with admin configs but issue did not solved.
Also i try with suggestion of @KarlShea, but this also not solving issue for me.

karlshea’s picture

karlshea
he/him
Primary language English
Location Minneapolis 🇺🇸
commented

@ravi did you change base in splide.libraries.yml to point to js/src/splide.base.js and clear caches after commenting out that line?

karlshea’s picture

karlshea
he/him
Primary language English
Location Minneapolis 🇺🇸
commented

I think I figured it out:

var go = function (el, source, dest) {  

  // This gets captured in scope, so isn't recalculated on each click which means that the current index is always zero.
  var index = instance.Components.Controller.getIndex();

  $.on(el, 'click.' + NICK, function () {

    // If it's moved here instead, it's recalculated on each click and seems to work as intended.
    var index = instance.Components.Controller.getIndex();

    if (index === source) {
      instance.go(dest);
    }
  });
};

That being said, it looks like this block of code is forcing rewind behavior. I didn't notice because I had rewind enabled in my optionset, but if I turn off rewind this code is still forcing it. Rewind is handled by the Splide library itself, so if that behavior is desired it can be enabled without this code.

@gausarts any thoughts?

karlshea’s picture

karlshea
he/him
Primary language English
Location Minneapolis 🇺🇸
commented

Looks like this was added in 3691c830: "Fixed for unwanted asnavfor clone styling."

ravi kant opened merge request !5

ravi kant’s picture

ravi kant commented
Status: Active » Needs review

Thank you @KarlShea
The issue solved for me and i have created MR !5.

gausarts’s picture

gausarts commented

Thank you

@KarlShea, you are correct.

It was a new 2.0.3 module feature to remove disabled arrows/ buttons specific for Slide, mostly related to Splidebox. Basically making Slide works like Loop, only arrows are the concern.

Unfortunately, poorly executed.

Honestly, I didn't know rewind was what I was looking for. My mindset was it was autoplay-related option :)

But if it does the job as you said, removing the offending line is the correct one. If not, your #4 is another correct alternative.

@ravi kant, libraries definition change was only for debug purposes IMHO.

karlshea’s picture

karlshea
he/him
Primary language English
Location Minneapolis 🇺🇸
commented

@gausarts reverted libraries change and removed references to the old function. There might be other things to adjust to fix the original issue you were fixing?

gausarts’s picture

gausarts commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Screenshot_2024-03-29-02-25-38-234_com.opera_.mini_.native.jpg329.24 KB

@KarlShea, thank you.

Removing the offending lines should be good. Hopefully rewind does the job, but not crucial.

OOT, I am just losing my Drupal permissions to give proper credits to your or others' authorships.

Previously I could tick you as an option for the author, but now I couldn't. I haven't got a chance to search nor report this issue to d.o. as it makes no sense if maintainers who should be able to commit and give credits to authors like you are no longer seeing you or others as an option for authors. I thought it was a temporary miss, but has been a while, though.

I saw Author column before like this:
https://www.drupal.org/files/GitAttributionUI_0.png
https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett...

But now I don't see it like in my screenshot.
Please allow me some time to report about this credit issue, or kindly shed the light in case I missed the obvious with recent d.o. changes.

karlshea’s picture

karlshea
he/him
Primary language English
Location Minneapolis 🇺🇸
commented

@gausarts I'm not too worried about it, but yeah that does seem weird. Maybe merging the MR will automatically do it? I'm not sure.

Although you probably need to build the minified JS anyways so the MR flow probably doesn't work with this module that well?

gausarts’s picture

gausarts commented
Status: Reviewed & tested by the community » Needs work

> Maybe merging the MR will automatically do it?
That's it. Just found the relevant issue:
https://www.drupal.org/project/drupalorg/issues/3177220

But then, gitlab MR appears to be unlinked from regular CLI commit pushes when not using MR select option. Not sure :)

Yes, we should update the minified, as well. Or you can use this online service which I normally use since mostly finer than offline ones: https://jscompress.com

karlshea’s picture

karlshea
he/him
Primary language English
Location Minneapolis 🇺🇸
commented

Unrelated to this issue, but after 10.1 Drupal can minify itself: https://www.drupal.org/node/3305725

Do you think it makes sense to remove the minifed JS from this module and let Drupal handle it?

gausarts’s picture

gausarts commented

Good to hear it, thank you.

When core has it builtin, we should remove our own minification. It will have more benefits like easy debug without hustles, and less maintenance.

Perhaps when min core requirements D10, or even sooner whenever I got a chance to update them since I can always update to D10 just as easily. Patches are welcome at another thread.

The only reason I minified all my JS is because I don't always use advagg, and core didn't have it till D10 :)

karlshea’s picture

karlshea
he/him
Primary language English
Location Minneapolis 🇺🇸
commented
Status: Needs work » Needs review

gausarts’s picture

gausarts commented
Status: Needs review » Fixed

@KarlShea, merged, thank you.
@ravikant, thank you anyway.

IIRC, this is my first time using a cellphone to commit an MR :)

There is always a first time for everything.

I hope I did it right, thus having credited @KarlShea alone since I can no longer assign author to choose one from two. Previously I could credit many, and chose one author, but not now, so I am a bit careful with this new UI stuffs till I figure it out any better.

Feel free to CMIIW.

@KarlShea, please confirm if you got your credit correctly.

gausarts’s picture

gausarts commented
Status: Fixed » Active

Oh, no.

As seen at #16 commit message, I credited @KarlShea alone, and it did merged @KarlShea's MR, only now authored by @ravikant?

I am sorry, I don't understand this MR selection stuff as predicted. The select options are not very clear to choose from even after correct credit.

We'll need a revert?

  • gausarts committed f100c2e4 on 2.0.x 
    Revert "Issue #3436319 by KarlShea: Thumbnail navigation arrows broken...
gausarts’s picture

gausarts commented

@KarlShea, I tried reverting this MR for my education purposes so to get the correct authorship credit.

And see if I can choose your new MR to get the correct credit about which I doubted if using MR !5 which was initially authored by @ravikant?

@KarlShea, I suspected you might need to create MR !6 or so on your own so to get correct authorship credit, not using MR!5 which was authored by @ravikant?

If not, do not hesitate to shed the light.
Thank you.

gausarts’s picture

gausarts commented
Status: Active » Needs work

KarlShea opened merge request !6

karlshea’s picture

karlshea
he/him
Primary language English
Location Minneapolis 🇺🇸
commented
Status: Needs work » Needs review

I'm not really concerned about the issue credit, but if you're trying to figure it out I found this on slack: https://drupal.slack.com/archives/C1BMUQ9U6/p1704365359171499?thread_ts=...

If you use the Merge button within the issue, it’ll. use the values set in the credit table, including the author.

Apparently it's a button within the issue below the credit table?

gausarts’s picture

gausarts commented
StatusFileSize
new Screenshot_2024-03-30-00-32-49-004_com.opera_.mini_.native.jpg332.21 KB

Hm, weird indeed :)

As seen in the commit message #16 I did credit @KarlShea alone, and under Merge I select the first option which was correct. But the author was still @ravikant.

However based on the previous #16 commit, I tend to think, the first author of an MR always got the authorship credit even though I have excluded @ravikant and my name from the credit table.

> I'm not really concerned about the issue credit...
Got it, thanks. I am just not comfortable to give incorrect credits with this new system :)

I merged your MR!6 now, but the system failed even though I had reverted the #16 commit. See screenshot and the previous system message exactly before this comment.

Maybe I'll need to use CLI later, but then I have no other way to put --author line for commit message copy/pasta as mentioned in the core issue above.

While I know you are fine with credits, however I am more concerned about the new system byproduct so that I could give proper credits later on. I will see if anyone had this issue, and commit it using CLI if anything failed.

karlshea’s picture

karlshea
he/him
Primary language English
Location Minneapolis 🇺🇸
commented

From what I've been reading, the commit author doesn't matter with regards to issue credits, that's why they removed the author radio buttons in the credits table.

gausarts’s picture

gausarts commented

> the commit author doesn't matter with regards to issue credits
My, this is truly relieving, but also shocking to me :)

Because then it was the opposite of previous system when authorship matters and determines the authorship credit.

The merge looks good now, thanks.

gausarts’s picture

gausarts commented
Status: Needs review » Fixed

I don't quite understand #26 :)

I thought it would say ... authored by KarlShea.

But I assumed it is the way it is, correct?

karlshea’s picture

karlshea
he/him
Primary language English
Location Minneapolis 🇺🇸
commented

#26 just means that it's the commit author, because I cherry-picked the squashed merge commit which had Ravi as the author. It doesn't influence issue credits.

This issue is showing up as credited on my user account, I think if you merge from the issue after picking users in the credits table that's all that matters now.

That being said, none of this is really documented or very clear anywhere. I'll have to be mindful of merging from the issue on some of the modules I maintain to make sure issue credits work right because I didn't know that was needed either.

gausarts’s picture

gausarts commented

Noted, thanks.

I'd love to embrace this new system to ease things up, but so far worried about giving incorrect credits as previously described.

Your comments help me see this new system better, although honestly I am still a bit confused seeing those MR commit messages conflicting my usual old CLI commit messages :)

karlshea’s picture

karlshea
he/him
Primary language English
Location Minneapolis 🇺🇸
commented

It is definitely an adjustment, after working with the new MR flow awhile I feel like I understand what's going on better but it still feels pretty awkward how things are spread out all over.

The other thing I wish they'd make way easier is syncing the base branch into the issue branch, it's always a juggle getting origin/2.x and nnn-my-issue-branch/2.x up to date and rebasing everything.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.