@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.
I tested this MR in my local and it's working as expected, thanks!!
Test is passing, thanks!
javi-er → made their first commit to this issue’s fork.
I left two small comments in the PR, but I tested it locally and it's working great! existing visualizations pointing to the public domain keeps working, and if I add another domain, it works well too.
After digging through the HTML markup of the navigation and the styles, I found out that the all: revert; CSS property being applied at admin-reset-styles.pcss.css to both the draggable elements and the dropzone element is conflicting with Chrome DnD API and making it not to work, simply removing this property fixes the issue. (
related issue
✨
Reset theme css.
Fixed
)
However, there seems to be some other issues being caused by the navigation styles (and maybe JS?) while on the LB interface. See discussion here.
The optimal solution is to identify and separate the Navigation libraries between the core styles and everything else and while on the admin interface, and just include a library that applies the basic styling leaving out anything that's not strictly needed.
Attaching video of drag & drop working after just adding forceFallback: true to layout-builder.js.
We considered the implemented approach but the problem with this is that when the menu slides to the right it covers the borders for a short amount of time causing some flickering, that's why the original approach used an :after pseudoelement imo.
It's a tiny issue and not very noticeable, so I'll leave to the maintainer to decide if further action is needed, see attached video.
Here's what I found so far in case someone else is looking at this, I debugged layout builder JS implementation and found that the onEnd event of the Sortable implementation isn't being called (layout-builder.js line 165) so after some searching and testing I ended up on this Sortable issue which is reporting something similar, some of the solutions suggested adding forceFallback: true to the Sortable call, I tried this and it started working on Chrome without issues. It seems to be related to a Chrome bug but I don't have a definitive solution yet.
Looks good to me, both functionality and code changes.
Before and after MR videos attached.
javi-er → created an issue.
javi-er → created an issue.
I opened a merge request with this change, please review it.
@shiv_yadav the problem with your patch is that it will still create a <ul class="none">, the solution is actually checking that below_type is different than "none".