- 🇮🇳India secretsayan
Marking this again as Needs Review as PHPSTAN is already passing in the pipeline.
- 🇨🇦Canada SKAUGHT
@ahsannazir thanks for the followup. hiding old branch to reduce confusion.
- 🇬🇧United Kingdom catch
. Are you suggesting that we follow that same approach wholesale, in which case this issue is not necessary
No not really, just the config management part. 📌 Holistic logo image insertion and dimensions in themes Active is an existing issue with the theme logo that we shouldn't re-introduce to navigation.
- 🇨🇦Canada m4olivei Grimsby, ON
I see now that a follow up was created for 📌 Store the file path instead of ID for the navigation logo Active .
- 🇨🇦Canada m4olivei Grimsby, ON
The theme logo UI lets you upload a managed file, but the configuration stores the file path, and it also lets you enter a file path directly. This means even if you rely on an uploaded file, it's possible to change the local configuration to point to the file path without having to know the specific file ID.
🤔 Great point. It makes good sense that the navigation logo would follow the theme logo UI more closely. Although the theme logo doesn't do any management of the size of the image, neither is enforcing a size on upload, adjusting the size on upload, or applying an image style on output. Are you suggesting that we follow that same approach wholesale, in which case this issue is not necessary? Or are you still OK with the approach here and wanting to add the consideration for better config export support in a followup issue?
This would then provide a way for site owners to use SVGs, which would partially solve #3436520: Decide if and how we should support SVGs on the Nav Icon then.
Agree it would help that issue.
The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇺🇸United States DamienMcKenna NH, USA
It's the end of July, 11.0.0-rc1 is out already, what's the plan here?
The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- @skaught opened merge request.
The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇬🇧United Kingdom catch
There's one other issue here, I didn't review ✨ Add a UI option to change or hide the logo on the Toolbar Needs work before it went in and just realised it now thinking about this one.
Requiring a file upload here means that this has to be done on the production site, and then the file entity and file itself synced back to a development environment, and then the config exported.
The theme logo UI lets you upload a managed file, but the configuration stores the file path, and it also lets you enter a file path directly. This means even if you rely on an uploaded file, it's possible to change the local configuration to point to the file path without having to know the specific file ID.
This would then provide a way for site owners to use SVGs, which would partially solve 📌 Decide if and how we should support SVGs on the Nav Icon Active then.
- 🇫🇷France nod_ Lille
I went through the same though process as you @catch and got the same "umm I don't know but I don't have a clearly better solution" conclusion. I should have commented, it could have saved you a bit of time.
I'll leave the commit to someone else too, this one is a bit out of my scope.