@javi-er

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!

@bronzehedwick I think this is good to go! I can confirm that the tabled library is being loaded and the selector can be configured.
There are some styling issues though that can be fixed in a follow up ticket (at least when I test this with a project with custom styles).
Great job!

javi-er → created an issue.

@bronzehedwick it looks great! I left a bunch of comments regarding semantic aspects, once those are resolved I'll test it in my local.

@penyaskito that's a good idea, I added it as a setting, default is false in order to not disrupt existing implementations. But I'm not sure if it should be displayed or hidden by default.

This needs review, I removed the settings for hiding the toolbar, if someone still wants to have the sharebar hidden for private server, the template needs to be overridden from a custom theme. In any case these settings don't affect public server, it seems like it's a bug on the Tableau end.

javi-er → created an issue.

@rmorelli can you specify the steps to reproduce this issue, what are you seeing and what should be happening?

javi-er → made their first commit to this issue’s fork.

The Drupal 7 version is not longer supported.

The Drupal 7 version is not longer supported.

The Drupal 7 version is not longer supported.

The Drupal 7 version is not longer supported.

The Drupal 7 version is not longer supported.

The Drupal 7 version is not longer supported.