@jonsimonsen, thanks for testing this again! that issue is indeed weird, if you see it again please open a new one, meanwhile I'm closing this ticket. If anyone find this problem again, please re open it with steps to reproduce and version of the module and Drupal, thanks!!
I see the add column and row buttons with Drupal 10.5.4-dev using the current version on the 1.0.x branch, I think we should do a new release and re test this since 3531224 is now merged.
@bronzehedwick I moved the function to the .install file since that's where it should go and I made some changes because it wasn't working well for me:
1- if ($key !== FALSE) to cover the cases where the button is found in the first position.
2- saved the array_values($toolbar_items) instead of the array as it was because it wasn't removing it for me otherwise.
Can you test it and see if it works well for you?
javi-er → made their first commit to this issue’s fork.
Hi, I wasn't aware that this issue already exist and I added support for Gitlab CI through another MR, thanks for reporting it though!
@leoenriquezp patch on #3 worked for me in my local, although the changes don't seem to persist when I save the node.
In any case I'm not convinced that this module should provide those options, we are removing some of the core table controls in order to improve accessibility and responsiveness and some of these options are a step back. Specially the width and height controls for cells and tables, the Tabled plugin will automatically apply a special class to cells to display them as narrow or wide depending on the content of the cell already, these controls will conflict with it. Table width is set to auto as well through the plugin, and if it needs more horizontal space controls are provided for navigating the different columns.
I think that if you need to have border, colors and these other options then it's likely that this module is not the best fit for that specific use case.
Although, the cell alignment options can be useful for editors I guess, for instance if you have a cell with numbers and you want those aligned to the right. Maybe we can include just this functionality without supporting dimensions, background or border options.
I'm changing the status of this ticket to "won't fix" for now, but feel free to re open it again if you have a compelling reason for including it.
I tested it in my local and it's working well!
javi-er → made their first commit to this issue’s fork.
The test page @q0rban mentions it's on the Tabled repository and not here, but we can include it in a future release in the vendor folder.
Thanks for adding and testing this, merged!
javi-er → made their first commit to this issue’s fork.
The available options are by column or both to improve accessibility, HTML tables with only row headers aren't described correctly by some screen readers, tables should always include column headers, even if you are using row headers. This is generally the recommended approach and considered a best practice.
javi-er → created an issue.
This looks good to me, we just did a release a few days ago, I don't think that this change is enough for making another release, but it's merged so it will go with the next one.
javi-er → made their first commit to this issue’s fork.
@owenbush thanks for the review! can you take another look please?
javi-er → created an issue.
Looks good to me!
@bronzehedwick the hook update is setting up large_character_threshold and small_character_threshold however everywhere else in the code is using large_character_theshold which has a typo, so the config is never read. The config field label has a typo too.
@bronzehedwick this is great! thanks for adding it, I think it will be very useful.
The code changes look great, however it needs a hook_update_N() → for setting up the configuration settings for existing installations, I tested it on a site that's already using the modules and the default settings are null. See attachments.
I did a functional test and embedding keeps working well, looks good to me! see attached screenshot.
javi-er → created an issue.
The iframe has a specific class, I adjusted the JS for checking if the iframe has this class before adjusting the height, in this way no other Qualtrics embeds will be affected. Since there are sites that have Qualtrics forms embed using this module and feedback plugins embed through GTM.
javi-er → created an issue.
Works great thank you!
javi-er → made their first commit to this issue’s fork.
@bernardm28 I like clean_unique_id as well since it adds an incremental value if necessary instead of a random number, but in terms of experience for the actual user it doesn't change anything since screen readers don't actually announce the id value, neither is visible in any way in the browser, maybe in the URL if you link to it.
@pdureau left some feedback at #3515766 📌 Create "statistics" component for Olivero in Drupal CMS Active that's also relevant for this component, I can help adjusting this component as well, let me know if I should work on this too.
Thanks for the feedback @pdureau ! yes, you mentioned sdc_devel to me but I completely forgot about it when I got to work on the component, instead I used as example the upcoming accordion component, which it's also facing similar issues now that I look at it (#3508546). I rushed it up to have it done before the contribution sprint ended, but I can iterate on this as many times as necessary until it's good.
When I try to see the sdc_devel report I'm getting this error, searching the issue queue
I found this one
🐛
Fix issue LogicException: Attribute "name" does not exist for Node "Twig\Node\Expression\GetAttrExpression".
Active
that's similar and should be fixed, I'll post an issue on the module later regarding this.
LogicException: Attribute "value" does not exist for Node "Twig\Node\Expression\Binary\ConcatBinary". in Twig\Node\Node->getAttribute() (line 158 of /var/www/html/vendor/twig/twig/src/Node/Node.php).
It makes sense to use the attributes variable, I was wondering about this when implementing it, also I like that the objective is to keep components free of Drupal specific functions, it should make them more compatible with external systems.
There is a reference to a definition provided by Experience Builder module, so this may break if the module (which is still in an early phase) is not activated
It actually breaks if the module is not enabled, I had to enable it in order for my component to render. Looking at the other existing components, most of them use these XB specific schemas, if the objective is for these components not to require XB, it should be fixed in almost all the other components under drupal_cms_olivero/components as well. I changed this one to use the regex you suggest.
description may be better as a slot, to be able to inject any renderable in the DIV
Agree, it's very likely that this text comes from CKE in the future, I changed it. I switched t a block for consistency, but I can use a variable instead if prefered.
Let me know if you have more feedback, this also needs a CSS review and a functional review. I tested with every combination I could try to make sure that it works well under most use cases.
@caligan yes I agree, "value" makes much more sense, updated.
@mherchel I adjusted the details from your feedback, thanks! let me know if you notice anything else.
Regarding the 'aria-labelledby' attribute, I added it to the section, but since headings for sections and articles are required, it shouldn't be necessary to have it, I've done some tests with 'article' elements and headings without it and they are described correctly by accessibility tools (unless it's nested under several elements or inside an anchor element), however it doesn't hurt to include it.
The term 'pretitle' seems appropriate as well for describing the short text above the statistic, naming things is hard. However I remember having this same discussion internally and 'eyebrow' is an old term, widely used in print media for describing these elements too. Let me know what do you prefer and I'll be happy to change it.
By the way, @caligan feel free to push changes to this MR if you feel too, I don't mind working on this collaboratively.
@caligan sorry about that, someone pointed me to this ticket and since it wasn't assigned to anybody I worked on it.
This is ready for review now, in order to test it you can do the following:
1- copy core/themes/olivero/templates/layout/page.html.twig to wthemes/contrib/drupal_cms_olivero/templates/layout
2- embed the component on page.html.twig:
{% embed 'drupal_cms_olivero:statistics-group' with {
title: '3 Statistics',
description: 'Microcontent description - I think this new service can be a good alternative in this case. I responded with comments for the question regarding the Secret Santa Party.',
cta_url: '#',
cta_text: 'Optional CTA'
} only %}
{% block statistics %}
{% embed 'drupal_cms_olivero:statistics-item' with {
eyebrow: 'EYEBROW TEXT HERE',
percentage: 'XXXX%',
description: 'Statistic description - I think this new service can be a good alternative in this case.'
} only %}
{% endembed %}
{% embed 'drupal_cms_olivero:statistics-item' with {
eyebrow: 'EYEBROW TEXT HERE',
percentage: 'XXXX%',
description: 'Statistic description - I think this new service can be a good alternative in this case.'
} only %}
{% endembed %}
{% embed 'drupal_cms_olivero:statistics-item' with {
eyebrow: 'EYEBROW TEXT HERE',
percentage: 'XXXX%',
description: 'Statistic description - I think this new service can be a good alternative in this case.'
} only %}
{% endembed %}
{% endblock %}
{% endembed %}
See attached screenshot, make sure to test with three, two and one statistic.
I think this is fine, I tested in my local and it works, although I had to tweak the example above a bit. In a future iteration it should be polished and CSS hardcoded values replaced by variables. I also tested with VoiceOver and elements are announced correctly.
Screenshot attached.
javi-er → created an issue.
javi-er → created an issue.
penyaskito → credited javi-er → .
javi-er → created an issue.
@penyaskito, thanks for the feedback! it's fixed now.
@penyaskito, this is ready for review again.
@penyaskito, this is ready for review!
@penyaskuto, this is ready for review including Gin accent colors and dark mode options.
javi-er → created an issue.
javi-er → created an issue.
The problem with @kasey_mx patch is that it replaces the build script entirely, so any new releases of this module will have the build code reverted to the specific version of this patch without you noticing it. Ideally a config option should be added for allowing or removing linking as asked above.
@bronzehedwick yes! sorry, I should have left a comment with the test steps, you should go to the configuration at `admin/config/content/ckeditor-responsive-table` and enter a selector that will not target any tables for instance I used `table.ignore_all` which targets tables only with the `ignore_all` class which doesn't exists. So no tables should get the plugin treatment.
Thanks for the review @bronzehedwick ! there was an extra issue that I fixed, could you take another look please?
Thanks @edmoreta ! we need a functional test as well.
javi-er → created an issue.
The string for getting and setting the table selector had the wrong format, I also removed some debug code from JavaScript that was outputting some messages to the console.
This is not longer an issue, it was fixed in a previous version.
The issue that was mentioned with styles at css/responsiveTableStyles.css that are referencing a figcation element that doesn't exists should be addressed in a different issue.
There is also another issue with the Tabled plugin which is that the navigation should be set to display: none when it's not necessary since it's creating some white space between the caption and the table when the controls aren't present, this should be addressed as a new issue on the Tabled plugin.
The problem was with the Tabled plugin, see: https://github.com/Lullabot/tabled/issues/7
When the plugin was created, I didn't include the check for the `hide-caption` attribute that the CKE button adds to the `caption` element which is an indication that the user has choose to hide the caption.
javi-er → created an issue.
@bronzedwick is this a duplicate of #3484482 ?
I'm also seeing the same issue with the latest version.
Looks good to me, I checked out this branch in my local and the Tabled plugin is being initialized correctly.
This is ready for review
javi-er → created an issue.
@bronzehedwick I merged the only change that was reported by this bot, the description says that if it's kept open it will keep adding MR with changes if there are other things that can be automated for D11, I would say that it can be left open until there are a few D11 projects using this module and then we can close it for good.
This look good to me!