smustgrave → credited nod_ → .
Committed and pushed 1c33460a8c to 11.x and f1139711e4 to 11.1.x. Thanks!
hang on, yarn vendor-update was not run on 11.x, lots of changes to the cke sources
we can get the max input var value and check with JS at submit for example. It's a pretty dangerous situation so special casing makes sense
hook_requirement make sense. i would prevent the form from being submitted with a warning if we know it will go over
nice thanks
The main issue is at the http/php level
@tedfordgif we have done all we can at the html level. no more cruft to remove. the shadow inputs are not submitted
yes makes sense to backport to 10.4
we can fix the uis where this makes sense, like the cell for tabledrag, the translation table, etc. not a by defa,ult thing
you're right, reverted
probably a false positive
The mr should target the 11.x branch
very nice, I like that a lot.
the list needs to be categorized (the query objects should probably be transformed to views in our case) and prioritized (based on the usage across the different wordpress block themes
this makes the bot crash, i don't have 10G or ram on it
we would need more info from the logs, watchdog or server logs to know what's going on.
Committed and pushed 94bc96150c to 11.x and 59e2130adf to 11.1.x and c5f9eea85c to 10.5.x and 3e4bcd62df to 10.4.x. Thanks!
Adding a bit more details to the format
Committed and pushed a11036104f to 11.x and a3941f583e to 11.1.x. Thanks!
issue summary is not up to date. We should probably open a different issue to remove the use of spaceless in core and leave this one to deal with the deprecation or polyfill for contrib is needed.
Can someone open an issue with the code from MR !9665 ? thanks!
It doesn't sound like this make sense to find an alternative solution. Let's just wait.
sorry wrong status change
So ios 18 fixed the issue, and we're still supporting ios 17 per our supported browser policy, so not quite in the clear
some simplification on the code needed. looks good otherwise.
We're going to need some tests to make sure a ckeditor update doesn't break this. See core/modules/ckeditor5/tests/src/Nightwatch/Tests/ckEditor5EditorHeightTest.js for an example on how to do.
Let's get some minimal styling going on. just some barebone flexbox layout to replicate the positioning we can see 📌 [PP1] Show entity information on the Top Bar Postponed
So thanks to testing by rkoller, scrolling is still an issue on iOS with this solution, unless we have a solution that works there too I don't think we should commit this
Started a change record and release notes. Feel free to improve them :]
Conceptually I don't see an issue. The fact that it's not editable makes sense.
on the PHP implementation i'm not the best person to comment, just pointing out that the region names feels a bit magic, if we're really tryin to enforce the 3 areas i'd use an enum or something a bit more restrictive (and maybe easier to document).
As it is contrib can add anything fairly easily, and i'm not sure it's a feature or not.
I can't seem to replicate the test failures. When I run the tests without the immediate and early return fix from the dialog.position.js file it's still green.
pameeela → credited nod_ → .
the block ui is going to be replaced by experience builder eventually and it's now trivial to make grid/columns with CSS.
Text needs some fixing, "you'll" should be replaced with "you will"
There is a merge issue to resolve
Committed and pushed 7b2bc5c1e4 to 11.x and 2dd4b0a0c3 to 11.0.x and 3f77afc88b to 10.4.x and 76a7be52b3 to 10.3.x. Thanks!
The supporting script has long be removed, nothing uses these attributes.
Committed and pushed 0ac15bb59a to 11.x and 187e64b5a1 to 10.4.x. Thanks!
Would like pdureau to check this
Missed a composer install before running phpstan.
Committed 2ae1c01 and pushed to 11.x. Thanks!
Doesn't cherry pick to other branches because of the phpstan baseline. Not sure we should backport, might make some patches less cherry-pick friendly.
RTBC +1, getting some phpstan errors locally when trying to commit. Probably a local setup issue leaving this to someone else for commit
oh but that would sort the machine names, not the labels...
I think we can make this simpler.
Adding a sort in \Drupal\Core\Entity\EntityFieldManager::getFieldMap fixes the issue
// In the second step, the per-bundle fields are added, based on the
// persistent bundle field map stored in a key value collection. This
// data is managed in the
// FieldDefinitionListener::onFieldDefinitionCreate() and
// FieldDefinitionListener::onFieldDefinitionDelete() methods.
// Rebuilding this information in the same way as base fields would not
// scale, as the time to query would grow exponentially with more fields
// and bundles. A cache would be deleted during cache clears, which is
// the only time it is needed, so a key value collection is used.
$bundle_field_maps = $this->keyValueFactory->get('entity.definitions.bundle_field_map')->getAll();
foreach ($bundle_field_maps as $entity_type_id => $bundle_field_map) {
foreach ($bundle_field_map as $field_name => $map_entry) {
// SORT HERE
ksort($map_entry['bundles']);
if (!isset($this->fieldMap[$entity_type_id][$field_name])) {
$this->fieldMap[$entity_type_id][$field_name] = $map_entry;
}
else {
$this->fieldMap[$entity_type_id][$field_name]['bundles'] += $map_entry['bundles'];
}
}
}
The result is cached so performance impact should be minimal. If we're worried we can also wrap this in a count() to sort only arrays with more than one item.
I don't think increasing the API surface for this is worth it.
We need more context in the translated string, whitespace rules around colons are different in english and french at least so we need to know a bit more than just ":".
Hide 'Read more' link in teasers
That's not quite it. The design has no links at all, no comment link, no flag link, no statistics link. Being able to toggle "Read more" is not enough to solve the issue.
The problem is that Read more is hardcoded to be added only on the teaser view mode. You cannot create a different view mode that has this Read more link and use that in the views configuration instead of the teaser view mode (which would solve the problem pretty nicely without a sub-theme).
We have 2 problems:
- Read more is hardcoded to teaser view mode, and can't easily be added to a different view mode (need a configuration at the field formatter level I'd say)
- The standard install add links to the teaser that Olivero has to remove in twig to match the design
I get the frustration that a seemingly simple solution is not accepted and merged to "solve" the issue. As was said earlier, the Olivero implementation used a workaround to match the design. Sometimes we have to say stop to the duck tape and solve the root cause properly, this is one of those times.
Committed and pushed 1bf08a6c435 to 11.x and cdc0427b3bb to 11.0.x and 40b402a1214 to 10.4.x and ffffbf3cd4a to 10.3.x. Thanks!
Committed and pushed 4bfa1e9987e to 11.x and 50692daf255 to 11.0.x and c2bdd141b48 to 10.4.x and a0b547ee0f9 to 10.3.x. Thanks!
Frontend framework manager here.
I agree that removing links forcibly from twig is not ideal, at the same time the design calls for no links in teasers, and that was confirmed several times over the years (#5, #61).
No links in teaser is a constrain we have to work with.
We can find solutions if we reframe the problem. What I got from the issue and all the comments is that this part is the real issue:
the Olivero theme is forcing the links to not be displayed, regardless of the field display configuration
We can totally reconcile the display and configuration:
- Don't explicitly exclude links from the node teaser (this is in the current MR)
- By default disable "Links" on the teaser display (this is not in the current MR)
That way we follow the design while making sure we didn't break expected functionality.
There are a few things to keep in mind:
- If it's disabled by default people might not be aware it's even possible to have when they change theme, making the links even less relevant maybe?
- How do we deal with existing sites? I can't support changing the default homepage of all the Olivero sites out there. There needs to be an automated way to prevent that, or at least copious release note and change record to explain to less technical users how to go back to what they had before (no-links)
We could also go another way and follow Claro/Gin: have Olivero in core and a "Better Olivero" in contrib to try more things out.
Before going further I'd like to have some ideas on how to deal with existing sites, knowing that changing all default homepages should be the very last solution.
Removing subsystem maintainer tag, we had that in #61,
Removing product manager tag, this is a technical issue, not a product issue.
Tagging for release managers for the config update part if it comes down to that.
There seem to be a couple missing (outside of @code/@endcode blocks):
core\lib\Drupal\Core\Extension (1 usage found)
ModuleHandler.php (1 usage found)
405 // specific variants of it, as in the case of array('form', 'form_FORM_ID').
core\lib\Drupal\Core\Form
FormState.php
1288 // self::getValue(array('foo', 'bar')), which is the level where we
core\lib\Drupal\Core\Theme (1 usage found)
ThemeManager.php (1 usage found)
427 // specific variants of it, as in the case of array('form', 'form_FORM_ID').
core\modules\file\tests\src\Functional (1 usage found)
FileFieldWidgetTest.php (1 usage found)
163 // iteration, so array(1, 1, 0) means:
core\modules\file\tests\src\FunctionalJavascript (1 usage found)
FileFieldWidgetTest.php (1 usage found)
96 // iteration, so array(1, 1, 0) means:
core\modules\locale (1 usage found)
locale.bulk.inc (1 usage found)
208 // Update the seek and the number of items in the $options array().
core\modules\node\src\Plugin\Search (1 usage found)
NodeSearch.php (1 usage found)
277 // array('type:page', 'term:27', 'term:13', 'langcode:en');
core\modules\sqlite\src\Driver\Database\sqlite (1 usage found)
Schema.php (1 usage found)
659 // The key definition can be an array($field, $length).
core\modules\system\tests\modules\common_test (1 usage found)
common_test.module (1 usage found)
96 // \Drupal::moduleHandler()->alter(array('drupal_alter', 'drupal_alter_foo'), ...),
core\modules\taxonomy\src\Plugin\views\filter (1 usage found)
TaxonomyIndexTid.php (1 usage found)
252 // Due to #1464174 there is a chance that array('') was saved in the admin ui.
core\modules\views_ui\tests\src\Functional (1 usage found)
core\tests\Drupal\KernelTests\Core\Entity (1 usage found)
EntityFieldTest.php (1 usage found)
252 // Test emptying a field by assigning an empty value. NULL and array()
core\tests\Drupal\Tests\Component\ProxyBuilder (3 usages found)
ProxyBuilderTest.php (3 usages found)
127 // @todo Solve the silly linebreak for array()
152 // @todo Solve the silly linebreak for array()
177 // @todo Solve the silly linebreak for array()
core\tests\Drupal\Tests\Component\Utility (1 usage found)
ColorTest.php (1 usage found)
166 // Input using indexed RGB array (e.g.: array(10, 10, 10)).
core\tests\Drupal\Tests\Core\Batch (1 usage found)
PercentagesTest.php (1 usage found)
39 // array(total, current, expected).
core\tests\Drupal\Tests\Core\ProxyBuilder (1 usage found)
ProxyBuilderTest.php (1 usage found)
42 // @todo Solve the silly linebreak for array()
not sure how much is in scope but they don't get picked up by the cli
I'll let others chime in on documentation, I do not know :D
As it is, the MR is ready to review and get feedback from everyone
Can I have screenshots for the comment section of an article? Olivero is not an admin theme so making the node/edit page looks good is not a priority. But if it impacts the comment form then it's a different topic.
Can you add one or two field to the comment entity to check how non-default comment fields are impacted by this MR? thanks!
@matthieuscarset: I see your point when you use Drupal as an icon library creator, then everything is better as content. That's not the use case addressed by this work. This is optimized for integrating existing icon sets into Drupal. It's still possible to use drupal as an icon library ceator and integrate with this new API though :D
As a sidenote we don't have permission access or translation of JS and CSS files (it's not the files that are translated it's what's inside) it's the same for icons so not having everything that content entities for icons does not mean we will need to reimplement everything.
PS: reading IS not clear how new API is supposed to be consumed by core, as contrib already has solutions
Once this is in there will be work to make navigation api use it it'll be clearer then.
Why can't icons be content entities?
Lifecycle of the data is different, a different object makes sense.
I'd hate to deal with icon pack update when everything is content, how do you start to write the update function for that in a way that works everywhere? Importing in content means duplicating the code for the icon, otherwise what's the point of having content you can't edit. And that opens the door to update problems if you've changed an icon and the source of your icons publish an update changing the same element how do you deal with that in a developer/user friendly way?
All that, or we can just realize that icons are not content and avoid most if not all those problems.
yep sorry bot was on the wrong branch
🎉
@pdureau, can you add credit to everyone that contributed to this so far on your side? Thanks!
Committed and pushed 390f87558be to 11.x and bddfccb8792 to 11.0.x and e7f6c9b5bcf to 10.4.x and 51a4a5fda1f to 10.3.x. Thanks!
Does it work when debug comments are present?
removing credit as requested.
couple of questions
This patch makes installing menu_test through the UI crash the site. It's not a production issue but it is an indication that the fix in kernel tests is not sufficient.
On the code side I reviewed and OK with it, just need this menu_test issue resolved.
will come back with more later today/tomorrow but there are still feedbak in the MR to address
Committed and pushed ba3a786beca to 11.x and 0d81ddd1910 to 11.0.x and 9be6e1c3cc7 to 10.4.x and c05d734567b to 10.3.x. Thanks!
Comming from
📌
Replace js message with webcomponents
Active
and in that issue it does esentially the same thing through a <template> tag added to the DOM somewhere (if we ignore the whole webcomponent/ShadowDOM part).
Instead of putting that in drupalsettings, I'd rather have a <template> dom node on the page to query when needed
Web components, +1 I don't think there is any concerns with using that in core beside the naming convention. For Shadow Dom (and especially declarative shadow dom) it's less clear. There are two topics
- style isolation and how much harder will it be to change from contrib/projects?
- Slots, the concept is great but it does come with potential issues
To elaborate on #2, when you have
<template id="umami-messages-template">
<h2 class="heading">
<slot name="title"></slot>
</h2>
</template>
<!-- then -->
<umami-messages-component>
<span slot="title">Something</span>
</umami-messages-component>
Accessibility picks up the h2, but it doesn't seem that you can access it from the JS. Just need to make sure we're all fine with that.
it's late so wrapping up here for today
Not sure how I feel about all this CSS for just that, but with one more css line it works.
thanks for the work, just wanted to say that the issue is on my plate but it needs a good chunk of time to review. It's on my todo but can't say exactly when I'll have the focus to tackle it.
Committed and pushed 9c0bd3775ca to 11.x and 562fa1c9b6b to 11.0.x and 9dd570757ea to 10.4.x and 229a850b786 to 10.3.x. Thanks!
all good thx
question in MR
Committed and pushed d44fb628e45 to 11.x and 0b0d880cbfd to 11.0.x and b60151a2656 to 10.4.x and 9b977fc3f84 to 10.3.x. Thanks!
yeah i'd rather have this in than not in
Thanks, solid work, still a few things to address. Thanks for your patience!
Ok, Happy with the JS changes.
For the test, I understand but i'm not comfortable using a sideeffect of the config UI to test an ajax regression. Can we make a more explicit test by trying to load the same file twice through add_js and add_css? It's a missing test so I guess that's why you had to create a new one: #3301769: Add test for the new add_js command →