lint issue
I'm seeing several issues related to using Olivero as an admin theme. it was never meant to be used that way.
@djsagar could you explain the use-case a bit more? I'd rather not have people spend time on making Olivero work as an admin theme, and instead work on Claro or Gin
Committed and pushed 903f73fb30 to 11.x and fbad2d37f6 to 11.1.x and 431532d69a to 10.5.x and 8c1d5599ef to 10.4.x. Thanks!
small change and it's good to go
Thanks for moving this forward @tame4tex, i checked the tests they make sense so +1 for them. Webform overrides some of the states API methods so any patch here will need an update on webform side
I replicated 🐛 Using a computed twig as a value for a visibility condition breaks in Drupal 10.3 Active and made a test locally and the MR fixes the issue. all good here.
For the JS I think it's solid, elements and event handlers are cleaned up so memory shouldn't leak. I wish we could make the code simpler but I don't have a better way (:
The scope is a bit too narrow, I think we can add to the theme/SDC maintainers.
we need @mogtofu33 to confirm reading the Drupal core governance, and agreeing to the responsibilities listed under:
and a MR :)
So in 11.x and yarn 2 we don't check dependencies because the command doesn't exists. From the docs:
NOTE: The command yarn check has been historically buggy and undermaintained and, as such, has been deprecated and will be removed in Yarn 2.0. You should use yarn install --check-files instead.
The --check-files option doesn't exist anymore.
the deduping doesn't impact our vendored deps so I'd be inclined to just ignore it and remove the check. i'll try to fix it later today but might just remove that
automation issue is at 🌱 Set up a formal process for ensuring JavaScript dependencies remain up to date Needs review
I don't see what to change in Drupal core, hence setting this as a support request.
The easiest way to go about this is exclude Drupal.t from being replaced. Using a library with attribute type: module shouldn't aggregate them so you won't get aggregation or minification for theses
Shouldn't that be called mergeResponse?
Porting credit
Committed and pushed e5a5bfe0d5 to 10.5.x and bd6031cb2f to 10.4.x. Thanks!
This will also allow themes to be much more reusable, making it possible to deploy a SDC-based theme configuration through a recipe
Added a link that explains a bit more of the framework https://fs.blog/playing-to-win-how-strategy-really-works/
Looks good to me
@nicxvan eventually we'll need to discuss specifics, but for now the focus is really on the overall strategy. First do you agree with the goal? it might not look like it but it's a pretty big change to become a CMS builder (and not "just" a site builder). Did we miss anything in the "where will drupal core play" section? any worries about the realignment of Drupal Core with regard to Drupal CMS, with the CMS market, or even with the broader open-source and commercial world. Does the "how will we win" section make use of all the strengths of Drupal in general, are we missing something to achieve the goal? I think it's more this kind of things we're looking for at this point.
The discussion about the specific APIs, render arrays, theme and hooks is to low level at this time. It'll be easier to discuss once we have a frame of reference we agree on to make decisions. If it's burning your fingers to talk about it right away, a new issue would be best.
Committer team gave some feedback in private that would need to be addressed before we can un-postpone
nice, didn't know we had that
Committed 3ef3cfe and pushed to 11.1.x. Thanks!
Cherry pick is not clean on 10.5.x, need a MR for it
Committed and pushed f99f198961 to 11.x and a67ab83394 to 11.1.x and 0aeedd16e5 to 10.5.x and 73487835e2 to 10.4.x. Thanks!
it's one string that needs translating, it'll handle all inline fields. No visible changes to users after this patch
The translation approach has been validated in #22, so we won't go with the CSS method. The string translation workflow is well oiled and adding a different way to do this doesn't provide significant benefits.
Here we need to add a context to the string, I suggest: "Field label"
we can add markup for this
smustgrave → credited nod_ → .
Committed and pushed 1c33460a8c to 11.x and f1139711e4 to 11.1.x. Thanks!
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.