@grevil: I was unable to provide the settings via JS drupalSettings, since when there were multiple instances of vidstack players on the same page, the settings of the first player were also used as the settings for all other players (usually correct, but it was messing up the differences in settings between local and remote players). By using data attributes as described in the vidstack docs injected via a twig template, I can fine-tune the settings for each individual player that is injected into the page. This will hopefully prove useful again, if we ever want/need to override the player settings on a per-entity basis.
I have now replaced all backwards compatible calls with the proper logger service via dependency injection. Would appreciate if someone took a look at it!
LGTM!
Part of our 11.x efforts, so it will be a follow-up to 📌 [META] 11.x feature updates Active .
Since this is part of our list of improvements for 11.x, we can simply tackle this as a follow-up to 📌 [META] 11.x feature updates Active .
lrwebks → created an issue. See original summary → .
All done!
Can anyone provide a newer link to the test results? Because the current one is not available anymore and as far as I can see, in the latest pipeline, the tests are green again.
@anybody, could you elaborate on your grouping idea? Specifically, how it would be done: Shall we group by date/time or detect duplicate entries that follow one another? Apart from that, I don't see many more ways to compact the design in a way that does not make assumptions about the message contexts that we cannot make.
I personally cannot reproduce the problem by the means that were described in the summary.
What I did:
- Run
drush cexwith Devel Debug Log enabled - Uninstall Devel Debug Log
- Run
drush cim
Result: The module was reinstalled perfectly fine.
When taking a look into my database overview (in this case PHPMyAdmin) I can also see that the table is automatically dropped just fine when uninstalling the module, even without our uninstall hook.
So I am not sure that the proposed resolution is actually the one to the problem that @killdery coelho is experiencing.
The only reason that I can see right now as for why Kint would not work and default to the Symfony var dumper would be that the Kint library is not installed on your Drupal installation.
You can see whether it is installed via composer show kint-php/kint and install (if necessary) via composer require-dev kint-php/kint.
If that is not the problem, feel free to reach out again.
It is also good to note that the Kint dumper is only a nicer and more structured way to view information about an object in most cases (meaning that it is not a necessity). The var dumper (that you are seeing right now) should also provide all information about the object, just differently.
Kint is also unable to show object methods anyway contrary to what you requested.
lrwebks → changed the visibility of the branch 3471860-add-action-to to hidden.
This is off to review again, and as far as I can see, done (implementation-wise)!
A last thing that should be done is to take into account different image styles for the local video poster image, so I will do that next before we can call this one done.
The formatter is in place and works as expected (as far as I'm aware). If a value is set in the formatter, it overrides any value from the field widget, but keeps the widget value if it is not set in the formatter. I also added a note regarding that in the formatter settings, so that there will be no confusion as to why a setting provided via field widget is not taking effect in the actual view (that confusion would definitely happen to me a lot :|).
So, it is off to “Needs Review” now.
I have added the help hook now and also rephrased the description and configuration sections to point out that the prefix label is generated, and the separator is configurable.
@grevil, what do you think — is the new phrasing appropriate?
The problem might be caused by the JS aggregation itself, since all JS is merged into a single file then, which can of course be the reason for the missing library files. However, @grevil told me that the code for these cookie tests was mostly taken from the “COOKiES” module, so we might need to have a look regarding the correctness there too?
We should have submodule specific sections, with their respective requirements, and what they do and how to do basic configuration.
In that case, we should opt to have a README for each of the submodules within their respective module folders, instead of cramming everything into here, right? At least, a lot of complex modules like Webform do it that way.
In a sense, I have already prepared us for this approach by keeping the main module description general and listing submodule features rather than explaining them in detail.
@grevil, what do you think about this? I will add the help hook in the meantime.
@grevil, the latest status of my work: I was struggling quite a bit with the AJAX integration when it came to dynamically adding / removing items from the settings form. After a chat with @anybody about it, we decided that my time was currently better spent on different tasks.
So, as of now, this task is open to anyone again, but it is likely that I will return to it if the priority of this rises in the future.
@grevil, the README is still missing, so I'll take care of that and then put the issue to “needs review”.
LGTM as a temporary solution, simple and effective.
The base functionality of the module is ready now and can already be reviewed, but I will keep this issue as "Needs work" right now since I will also write a proper README.md next.
@anybody I didn't entirely start from scratch, most of the code is initially taken from the Plyr module and then modified to fit Vidstack, but also some entirely custom things. I figured that this would be the best approach, since we were discussing to take the Plyr code as a starting point. On the other hand, there were lots of things that aren't needed for Vidstack, so I went with cherry-picking.
joachim namyslo → credited lrwebks → .
Okay, we're using the built-in default empty option now, just like the actual field in the slide config form does.
Heads-up why I am using this line here:
'#empty_value' => '_none',
If I do not provide a specific value for the empty option, it will disappear forever once the form is saved with a different value (e.g., “Ken Burns Effect”). But since we need it to stay, so I just passed its own default value to it.
Basic install/uninstall test is actually already there, as the CommerceBetterProductVariationLabelGenericTest.php file is present.
Once this is merged, I can use the README as a template for the module page and improve it a bit (including our support snippets at the end of the page).
Once this is merged, I can copy the README to the module page (changing it up a bit of course to better fit the webpage setting).
The proposed README is still missing a lot of points and not following the Drupal Standard → very well, so I will redo it quickly.
FYI: The summary looks like it does in the attached image now.
Found it: @thomas.frobieter, I think you meant the overlay_position fields? They do have a “no overlay” option, which is already present in the default settings form.
I have added the none value to the image_animation field now in a new branch, but as far as I'm aware, there seems to be no option named “no overlay” for the overlay_display field, even without our new changes. @thomas.frobieter, could you clarify?
joachim namyslo → credited lrwebks → .
The description of the fields in question indeed provides a link "Change default value" which leads to a slick optionset edit form. So we could just rename it to something more obvious like "Inherit from Optionset", so that everyone is in the know.
Update: This issue might be very close to fixing the problem: #2808063: LibraryDiscoveryParser::buildByExtension() doesn't validate that extensions exist →