[meta] Drupal 8.x Version

Awesome, thanks.
Could you provide more info on what parts you have ported? And what is left to do.
I am also open to discussion how to best deal with sub-modules and other things before we release one.

Cheers,
gaus

Thank you.
I realize you are still working with it, could you provide more info, or your strategy on the following:

• How "to integrate the views submodule into the main module"? Or the other sub-module?
• The skin conversion

Thank you.
Yes, I am with you, plugin is the way to go, and sub-modules dropped in favor of plugins.
But I am afraid I can't make it thru irc. I prefer writing it down ;) Hope okay.

BTW, I have started the conversion some time ago myself, but never really polished as the efforts were done at some luxury time, like everyone else here I think. And I got no energy to chase the head either.
Seeing that you got some energy to do the conversion, I am optimistic we can make it ;)

So I compared our works. Since you know better where you are now, allow me to outline what I did so far instead, in the hope that we can channel and focus the energy to make better what was already done, perhaps rework as needed, and finish the rest of TODOs to our best efforts only if the first stab is at my end.
However I am open if you care to provide your patches to put your work as a first stab.

The main point is we can make use the best of our luxurious energy to this coordination. This shouldn't be so luxurious if you got funded ;)

Below is what are partly done:
- The CTools optionset is converted to CMI, still need proper schema, also for responsive settings.
- Slick Image is JustWorks.
- Picture to core Responsive image is not really tested, but JustWorks.
- All theme preprocess in place.
- All templates are Twig, including most supported classes.
- Slick example module created since I was tired to rebuild slicks at every beta upgrade ;)
Again the disclaimer is most of them need more love.

Below is what are no joy:
- Slick Views was only working at Beta3, then not.
- Slick Media is 0, perhaps Media Entity is the way to go?
- Slick Field collection is 0.
I think we shouldn't worry much about non-existent-yet modules in the mean time.
- and many more i don't really remember now.

Please bear in mind, I made the above notes so that you don't really have to spend much energy for what was done. However if you think you want to start fresh, I am happy to support the efforts.

What do you suggest?

I am still coordinating with the main maintainer before making decision to push anything.
I have contacted him to this thread, so hopefully he can throw some light here.

Great to see others interested in the D8 port, too, I was just considering starting it myself as well (perfect timing here). :)

I am just starting to build a site that will need Slick as a major component, and I'd rather use this module than plain JS library (as we'll need it in the future as well), so I will be able to put some working hours into this project.

I will have an opportunity to look at and test the current version on GitHub in a few days, so I'll get back to this when I have some input.

Awesome. You can start provide patches, and let's review together.
Thanks.

Thanks, please find my comment inline:

is it okay when the patches are not clear?

No worries, I make a mess myself ;) You can clean them out later, lots of tools available to make them clean if you are not comfortable with manual work. That is why I said we can review together. But right now, nothing to review and worry about, so feel free to give the best shot whenever you are ready.

Should i create issues for each patch?

We are talking about Drupal 8 port. I recommend you start patching from 7.x-1, as it is simpler. Later we can do individual patch to address each issue.
Other reason is I haven't got an update from the maintainer if he is currently working with it, maybe he is still busy. But I prefer to think no news is a good news, I meant no news is a go-ahead ;)
So starting from 8.x-1 is a good start than jumping to 8.x-2 leaving an empty branch.
You may want to create another issue with proper version to patch leaving this to global discussion.
What do you think?

You mentioned before that you got a testing module? Could i get a copy of this?

I did. However since you didn't reply to my previous comment, I assumed you want to make first stab. However feel free to let me know if I should provide the first stab. As said, I am okay with your decision.

Could i get a copy of this?

Let's use the patching to start the conversion, not zip ;) I am not happy with my own work now, so I can't give it, sorry. As said still need some love ;)

Maybe we should write some phpunit tests too (rendering stuff etc.)

Awesome, let's do it later, when all is settled.

Cheers,
gaus

Awesome, thanks. I appreciate your contribution.

I think we should consider to create another issue with the version 7.x-1 since we intended to start from 8.x-1 as I said above:

You may want to create another issue with proper version to patch leaving this to global discussion.

What do you think?

But that should not stop you. Things happens ;) We can always use this to have a start for early review and discussion, and move it later to another issue with proper version 7.x-1. Just take your time ;)

Allow me some time to review this properly.
However this is a quick manual review before a proper one:

slick.info.yml
package: slider
Rather than "slider", perhaps "Media" is more related to the parent route name since sub-modules are dropped.

slick.libraries.yml
1. I don't think you need to use "jquery.slick", just "slick" is fine. The folder name is just "slick", and no other library offered so far.
2. Please split libraries, core library (slick.min.js) from module js (slick.load.js), so advanced users can disable the module JS as needed without too much hustle. Also I think respecting the orders ensures the library loaded first, but it is easy fix with proper dependencies.
3. You put non-minified slick.js with slick.min.js in the same library. If Drupal 8 doesn't supports auto separation, both files are loaded. Please let me know if any info I missed about this. Otherwise please remove one.

slick.routing.yml
Perhaps you should create "slick.permissions.yml", and port "administer slick" accordingly. Using "administer themes" to administer is a bit problematic.

slick.optionset.default.yml
Please change boolean values to lowercase.
If using drush, you may want to use "drush config-export" to avoid manual writing to get consistent array output with the form. The same structure applies to schema.

slick.theme.inc
1. You can safely drop all "template_process_" variants.
2. You may want to move some classes to Twig as you said before.

Drupal\slick\Entity\Optionset
This is arguable, however using "Slick" is more precise than "Optionset" IMHO, that is using the module name itself. We don't offer other entities, at least so far. You may want to see "Block" entity. Even if we want to add other entities in the future such as SlickContent or SlickDataset, it doesn't have to bother.

If you don't mind, please create another issue with 7.x-1 version so that the port will address the discussed 8.x-1 above. Drupal wants the port to have the same version with the issue IIRC, I just forgot the link to confirm.
Also please set Status to "Need Review" so we can properly change status accordingly.

I will need some time to have a proper review.
But I think that will be all for now. Thanks again for contribution.

Before you wonder why breakpoint is broken, please see the available JS options at:
https://github.com/kenwheeler/slick

You changed "responsive" to "breakpoint" at many places. Although you might think it is a good consistent naming, but I am afraid, that is not supported by the library.

Thank you. Less basic issues now ;)

As the breakpoints object is not contained in the settings object we have to manipulate it anyway. So breakpoints is a better name for an object storing many breakpoints. responsive on the other hand is more or less a name for a boolean variable.

I don't see it yet at your patch so it is not so clear what you meant.

However just in case we are not at the same page:
- responsive (breakpoints object) is indeed contained within the main settings.
- each responsive object has a breakpoint and its own settings.
- these responsive objects forms are generated dynamically using AJAX, which I don't see at your port yet.

Assumed this part is still a WIP, so let's see the implementation.

What is exactly the difference between the 1.x and 2.x branch?

Basically few things:
The 2.x branch extends 1.x with CTools exportables, the codebase is separated into sub-modules, all user settings are stored as JSON object within the slick container, skins, and extra optional integration: picture, colorbox, color field, media.
Both supports field collection.

Both do have a stable release.

No.

Do you wan't to add features to both version?
Only the 2.x branch has extra features, and I am done with it last week. Other 3 nice-to-have things are no big deal.
I have halted the 2.x branch development for now, unless for bug fixes.
At least to allow us port it to D8 with the latest version accordingly. So feel free to merge the codebase accordingly.

I noticed you still use Slick 1.3 codebase, but the latest one (Slick 1.4) is a bit different.
Be sure to update your codebase if using Slick 1.4 to avoid some JS errors in your tests, and most of all make the patch apply cleanly if you stick to patch 7.x-2. See below.

Is there a proper reason to migrate both version to D8?

I never say both versions should migrate. A misunderstanding, I think.
It is a matter of:

Rather than _starting from 8.x-2, we'd better start from 8.x-1.

No need for both versions. It will be too much energy to spend for anyone by now.

You can use the codebase from 7.x-2 to start patching for 8.x-1.
But I myself will patch 7.x-1, and use codebase 7.x-2, for 8.x-1.

Why?
The only reason is Drupal wants the issue version to be the same as the port, that is why I suggested to create another issue for version 7.x-1, however you may use codebase 7.x-2 to patch 7.x-1 for 8.x-1.

I think we still have some things to do before we can push it:
- plugins for sub-modules, at least image and views for the first release.
- drop slick/includes contents, and move them to methods with dependency injection accordingly. Some should go to internal, some front end.

You already moved some slick/includes contents, which is good, thanks.

I understand the pain.
I spent 8 months so far to make those changes, only never complaint when Slick changed much at Slick 1.4.

Please bear in mind nobody asked you to do it ;)

I am happy to welcome some good energy. And since you persisted I also welcome you to provide patches.
I even provided alternatives to the workflow to ease up your life above, but you might miss them.
Unless you are being so painful, you should not stop what you have started.
But no obligation whatsoever. It is open source.

Only if you still got some energy to move on, feel free to create another issue for 7.x-1, and leave this to [meta] discussion as intended at your OP [meta] Drupal 8.x Version.

That is okay with the basic features, if you think views and image is not basic.
Everyone else, and me too, can chime in along the way whenever available.

It's open source but you don't need to get extra stones in your way.

Sorry I don't get it. What did you mean? It is like "marriage is a two-edge sword"?

I see. Sorry but I thought you sound like the Mankoff guy I read some 15 years ago.

You don't actually understand what you are saying because either you didn't read your first comment, nor my comments all the way down.

If you read from the top to the last comment, I never push you. I am open to whatever makes it easy for you without further ado after knowing we are on the same boat, as I believe everyone will make things better over time.

I never thought patches are barriers. Nor views nor images are barriers, because I already did most of them, only not happy with it yet. Sorry if they are to you. No pushes whatsoever.

Anyway, no big deal, just a bit surprised after all my welcoming and encouraging comments, you came up with that stones. Perhaps because we are no native English.

Feel free to move on.

Regarding stones:
The stones were removed the minute you asked.
I have said "Sorry", "No pushes", "That's okay", "We'll chime in whenever available".
Anything else I can do to make you happy?

Regarding overblown patches:
So far I can't complain, and had provided fair reviews. You certainly don't need to review your own patches.

Regarding double job:
I offered you help to avoid this situation in the first place.
You were even suggested to start patching from 7.x-1 as discussed one day before you submitted the patches.
You were welcome to provide patches after I realized you ignored my offer.

I am a lucky Window user, and it only takes a few click away to patch thousand lines of codes across different deep folders.
Patching between different branches are less than a handful of click jobs for PC users.
I don't quite understand why would you make "double job" and "patching" sound so serious?
It should be a fun job that you do at your free time like everyone else. And you can pick whatever you think is best at your own conscious decision to patch anytime.
You don't need to submit patches till you are happy with your own work. Nobody pushes you to submit patches.

I said "whenever you are ready", "just take your time".
Is there anything I am not aware of about your workflow that puts you so much in pain?

Could we please get back on track?
If my previous reaction was jerky, I apologize.

You are very much welcome if you want to contribute.
But again no obligation whatsoever. I am beginning to worry about you.

Let's move to the implementation, and mark this fixed by the more relevant branch of D8 Port issue, that is, starting from the 1x, rather than jumping to 2x, since 1x is non-existent yet.

In the mean time, let's close this to focus more with the on-going work at the new issue here.

Further suggestion and improvement on D8 port can be separated issues.
Thank you.