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 →
@thomas.frobieter: Is this “Default” option redundant now? If it was just there to programmatically use a “default” styling that is consistent across the whole page, then it is no longer needed, as the new settings form does exactly that (but better :P). Or is this a regular setting just as the “Yes” and “No” options and has to stay?
Fixed the test, everything should be working now.
All three tests are added now, however the last one isn't fully working yet since I struggle to find the correct permission name for a user to access the DBLog page. @anybody are you in the know?
What tests are left to do after the merge of 🐛 Colon identified as a spam character Active :
- Language Script validation
- Reject message changes
- Logging rejected submissions
It definitely was a good idea to do some manual testing, because I found out that the trimming of the patterns on Form save didn't work correctly and broke the validation after. It should all be working now!
Done.
Working now, the summary items are only shown when the setting in question is not the default value.
Done.
I will also add a test for creating a custom token type before this is reviewed.
@anybody But isn't it a bit sloppy for the first test (the one that is testing the form functionality) to rely on the UI result instead of checking actually saved data?
I am not really able to move forward with this, as I don't really get behind the logic of how the custom tokens are stored. In the DB, it is sitting as a regular content entity in a separate table “token_custom_field_data” and also as the only one of its kind (since the test only creates a single custom token). When I try to load it as I would load a content entity:
$custom_token = \Drupal::entityTypeManager()->getStorage('token_custom')->load(1);
it simply doesn't work. I have researched a bit and tried different ways of loading the custom tokens, but it always returns `NULL`.
Does anyone have an idea as to what I'm doing wrong here?
This behavior is also implemented here: https://git.drupalcode.org/project/smart_trim/-/blob/2.x/src/Plugin/Fiel..., which is why I reused this existing logic for the less wrapper class in ✨ Allow selecting client-side or server-side trimming Active .
I agree with @anybody, that we should wait for maintainer feedback first, but this also looks prone to errors to me.
I just saw that this is only the case when using the “Smart Trim Client Side Formatter” introduced in ✨ Allow selecting client-side or server-side trimming Active , where this issue is already noted, so we can only start looking into this once the other issue is done.
Pipeline fixed!
Okay, perhaps the pipeline failure isn't that unrelated, I will take a look.
The base functionality and test coverage is there now, PHPUnit seems to fail due to an unrelated test from a different commit.
The machine name field already works on TokenCustomTypeForm.php, but not on TokenCustomForm.php. @anybody suggested to me that he will take another look at this. For now, back to “Needs work”.