- 🇺🇸United States greg.1.anderson
Updated the change record → to reflect current targeted Drupal core versions.
- 🇺🇸United States greg.1.anderson
Hum. Seems to still be up-to-date to me. :shrug:
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 greg.1.anderson
Thanks, Needs Review Queue Bot! Rebased to HEAD of 11.x branch.
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.
- First commit to issue fork.
- 🇨🇦Canada SKAUGHT
we need to make sure the image is v/h centered in a 40px A tag that wraps the custom image used. remember 📌 Create Image Styles for Nav Logo Active is also coming soon, images will be scaled down.
- 🇺🇸United States greg.1.anderson
Switched this to deprecate sql_mode in 11.1.0 for removal in 12.x, and backported sql_mode_options to the 10.4.x branch without deprecation of sql_mode.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇺🇸United States greg.1.anderson
greg.1.anderson → changed the visibility of the branch 2939760-10.3-allow-granular-overriding-of-sqlmode-options to hidden.
- @greg1anderson opened merge request.
- 🇺🇸United States nicxvan
At a quick glance that looks right: also if you want to link an issue in a comment where the issue status color comes up can you use:
[#issuenumber]like this ✨ Add support for tables with two headers in views table display Needs work
- 🇺🇸United States theMusician
Thanks for the feedback. I agree that the undefined array key error is because the views did not get the update to set grouping_label_element to null.
The ViewsConfigUpdater code requirement makes sense to me as being necessary. On a separate issue, https://www.drupal.org/node/2770835 → , the test update never fires even though I wrote a function in ViewsConfigUpdater but I was not aware you have to also call the function in updateAll(). The documentation improvements for how to use ViewConfigUpdater is a great idea.
Here is a rough idea of what I think is needed. @CarlyGerard also involved with this ticket walked me through what this may end up requiring. In the protected function addGroupingLabel() I can't quite figure out what the handler represents. The parameter defines it as a display handler, so I think a view style is represented here as the handler_type "style" and that is also the plugin_id? These ideas came from one of the issues you referenced, thank you for the pointer - https://git.drupalcode.org/project/drupal/-/blob/96d21a4603df23080a34051....
/** * Performs all required updates. * * @param \Drupal\views\ViewEntityInterface $view * The View to update. * * @return bool * Whether the view was updated. */ public function updateAll(ViewEntityInterface $view) { return $this->processDisplayHandlers($view, FALSE, function (&$handler, $handler_type, $key, $display_id) { $changed = FALSE; if ($this->addGroupingLabelElement($handler, $handler_type, $view)) { $changed = TRUE; } return $changed; }); } /** * Add Grouping Label to views without one. * * @param array $handler * A display handler. * @param string $handler_type * The handler type. * @param \Drupal\views\ViewEntityInterface $view * The View being updated. * * @return bool * Whether the handler was updated. */ protected function addGroupingLabelElement(array &$handler, string $handler_type, ViewEntityInterface $view): bool { $changed = FALSE; // Add grouping label element to existing views. if (($handler_type === 'style') && isset($handler['plugin_id'], $handler['type']) && $handler['plugin_id'] === 'style' && $handler['type'] === 'Grid' || 'HtmlList' || 'GridResponsive' || 'DefaultStyle' && !isset($handler['style']['grouping_label_element'])) { $handler['style']= ['grouping_label_element' => NULL]; $changed = TRUE; } return $changed; }
You asked, why the allowed grouping elements would not be restricted to just heading tags like the views pager update. For the grouping use cases, grid, HtmlList, GridResponsive it seemed hard to predict that one would only ever group by heading. Using labelELements, https://git.drupalcode.org/project/drupal/-/merge_requests/7351/diffs#11..., makes it so available elements do not need to be maintained in this code as well as the views.settings. I concur that the most common use case would be headings but I am not confident that there would never be a need to group by another element.
Thanks again for the feedback, the questions, and the guidance.
- 🇪🇸Spain ckrina Barcelona
-
ultrabob →
committed a3badee0 on 8.x-1.x authored by
kelly.m.jacobs →
Issue #3443802 by ultrabob, kelly.m.jacobs, wylbur: Add Gitlab CI
-
ultrabob →
committed a3badee0 on 8.x-1.x authored by
kelly.m.jacobs →
- 🇯🇵Japan ultrabob Japan
Workflow Buttons now passes all linting and code analysis with no warnings or errors. There is another issue for adding tests, given that the existing tests were not testing Workflow Buttons features.
- Open on Drupal.org →Core: 9.5.x + Environment: PHP 8.0 & MySQL 5.7last update
1 day ago Waiting for branch to pass - 🇺🇸United States phenaproxima Massachusetts
I noticed a few more things but this is looking very close to ready. It does still need a change record.
- 🇺🇸United States greg.1.anderson
Regarding the last change I pushed, while it would be possible to simplify the logic slightly, I wanted to point out that I deliberately ordered things the way I did to minimize the number of lines that had to be touched when the deprecated sql_mode support is removed.
- @plopesc opened merge request.
- 🇪🇸Spain plopesc Valladolid
Added a first POC of the new approach.
It will require some adjustments and tests, but I think it is working as expected.
- 🇺🇸United States smustgrave
Believe the changes addresses the concern by @bnjmnm and still addresses the issue.
LGTM
- 🇺🇸United States smustgrave
Thanks @quietone
I'm also available on slack in #contribute or #first-contribution to further help.
- 🇨🇦Canada SKAUGHT
yes, a correctly defined width and height attributes with fallback to svg viewBox if defined. i had mocks on that on other branches but we didn't have a sanitize routine worked out there.
just a reminder also we do need the correct size for the img as the logo is eager loading. we do output the correct image size from existing routing with a custom image.
(: lol. so much work for such a small picture
- 🇬🇧United Kingdom catch
Agreed that image style is tricky. SVGs definitely would need their own code path if added eventually, as I found out in 📌 Holistic logo image insertion and dimensions in themes Active - getting SVG dimensions is very different.
- 🇨🇦Canada SKAUGHT
u mean something like:
use Drupal\Core\Image\ImageFactory; ... $image_factory = \Drupal::service('image.factory'); $image = $image_factory->get($file->getFileUri()); if ($image->isValid()) { // Scale the image to fit within 40x40 pixels, maintaining aspect ratio. if ($image->scale(40, 40)) { // Save the scaled image. $image->save(); ....
- 🇪🇸Spain plopesc Valladolid
Good points @SKAUGHT
Let me try to clarify the approach to try to address your concerns.
The automatic resize image approach does not require the image module to be enabled because the image manipulation API is in
\Drupal\Core\Image class
.
If the Image manipulation API fails to transform the image due to the unlikely scenario where GD is missing or any other internal fail, we can show an error indicating that image has to be of the specified dimensions, likeFileImageDimensionsConstraintValidator
does.
If SVG support is added in the future, we might need to create a specific path for that - 🇨🇦Canada SKAUGHT
plopesc
the precursor problem is Image (core module) is enable and Servers have php's GD Library Enabled. As i've thought around this: we should just enforce a 40x40image and not offer to resize. And 'IF' down the line we allow svg --> same only verify size, let the user use the correct file in the first place. - 🇪🇸Spain plopesc Valladolid
Been thinking about this issue and I think that using Image Styles here adds an extra layer of complexity that might not be necessary.
Having an image style we need to assume these risks:
- Image style could be modified by site administrators, leading to unexpected results
- Image Style could be deleted by site administrators, so we cannot assume that it is present and need to have a fallback in every place we load the custom logo image
- Would force to add anew dependency on
drupal:image
for Navigation
My proposal here is to use a similar approach as we already have when uploading a picture to an image field that exceeds the maximum allowed size for the field.
When image is bigger than 40x40px, it would be automatically scaled during upload and a message like this would be shown to the end user:
The image was resized to fit within the navigation logo expected dimensions of 40x40 pixels. The new dimensions of the resized image are 40x40 pixels.
Following this path, code would be much simpler, reducing the number of possible points of failure and we would be safer assuming that original image can be used directly as navigation logo.
To give more flexibility, the icon size could be set as properties in the navigation.settings config entity, but not shown. So contrib modules or advanced site administrators could modify them.
- 🇪🇸Spain plopesc Valladolid
Removed a couple of unnecessary dependencies from test class.
I think it is safe to mark this as RTBC now.
Thank you for your efforts here!
- 🇳🇿New Zealand quietone New Zealand
@skessler, Just after the Issue Summary there is a 'show commands' link which while display information about the fork and branch. And, there is a how to for rebasing a merge request → as well. Linking that in case you need it. I hope that helps.
- 🇺🇸United States skessler Denver
@smustgrave, sorry for the basic question but I am trying to do the rebase I cannot find the correct name for the remote branch. I would think it should be origin/issue-3044974 but my local git client is reporting that the branch does not exist.
Thank you for the help.
Thanks,
Steve - 🇦🇺Australia sime Canberra
So i double checked and for me it's clickable in Safari.
- 🇺🇸United States greg.1.anderson
Updated the MR (11.x branch only) to deprecate the feature in 11.x, and remove in 12.x. Question: for the deprecation message, should it be version 11.1.0, or is 11.0.0 ok?
- 🇫🇷France nod_ Lille
Committed and pushed 6fc1e6f2e9 to 11.x and 1f7b5b3b4c to 11.0.x and b17eca1c91 to 10.4.x and 590fb5278d to 10.3.x. Thanks!
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 States bernardm28 Tennessee
This issue worked as expected on DrupalPod on Chrome.
We should open an issue up if it does not work in Edge and Safari but because this is an enhancement. I think supporting edge and safari would be could be a follow-up.
The feedback and accessibility concerns are valid and will be super useful as we make later enhancements but for now, I think this achieved the targeted outcome. - 🇺🇸United States smustgrave
Thanks sorry I should of been clear about the tests that was looking for that vs testing css changes.
1) Drupal\Tests\navigation\Kernel\NavigationMenuMarkupTest::testToolbarButtonAttributes //li[contains(@class,'toolbar-block__list-item')]/a[@data-icon-text='ti'] Failed asserting that 0 matches expected 1. /builds/issue/drupal-3424744/core/modules/navigation/tests/src/Kernel/NavigationMenuMarkupTest.php:159 FAILURES! Tests: 1, Assertions: 6, Failures: 1.
Shows the coverage.
Applied a super nitpicky change directly.
Believe this is good to go.
- 🇺🇸United States chrisfromredfin Portland, Maine
Two things at a minimum -
1. Let's remove the boxes at the right hand column, and just stack that information up with some decent spacing.
2. We really want to show the "summary" of the body field, then the images, THEN the full description. I'm not sure if there's backend work needed here to make that happen, but we should have access to the summary field separately from the full description. The summary is what we're showing on the cards view, so we want to repeat it, then show the images, then the full description.I think there are some other improvements to be made:
- Get rid of the "Details" heading
- get rid of the "Categories:" label and just show the chips
- let's get rid of the module author (i.e. "By Dave Reid") altogether
- let's make the slideshow actually work?? 😬 - 🇺🇸United States smustgrave
smustgrave → changed the visibility of the branch drupal-3424744-drupal-3424744-3424744-on-collapsed-sidebars-off-11.x to hidden.
- 🇺🇸United States smustgrave
Thanks @omahane
Test failure is unrelated was fixed when 🐛 After deleting a translated article, search wants to reindex it RTBC was reverted.
- 🇩🇰Denmark ressa Copenhagen
I created a separate issue for Drush
archive:dump
integration. - 🇨🇦Canada SKAUGHT
#32, indeed there are some open conversations around this.
📌 [meta] Improve the navigation layout page Active maybe the best issue to point you to.
- 🇦🇺Australia sime Canberra
I think the basic click ability is kind of useful in this case (it felt like a good intuitive improvement) and could be enhanced in follow up issues, but I'm intrigued why you couldn't click... i'll review again.
- 🇩🇪Germany rkoller Nürnberg, Germany
I've applied MR480 and also cleared the caches. Hovering over a category pill brings up the pointer, and adding the pointer rule to each list item instead of the wrapping unordered list seems like a reasonable call. only problem at the moment it doesnt happen anything on click, i've tried in safari and edge, while the click on the details page still works. but clicking a category pill on any card on the main page has no effect.
but there is also the question how the category selection behaves on click. if you have
access control
andaccessibility
selected in the filters category section on the main project browser page and you go to a details page and then click the categorydecoupled
for that module you are forwarded back to the main page,access control
andaccessibility
are unticked now whiledecoupled
gets ticked.
but the question is what would be the expectation, returning to the example from the details page, ifaccess control
andaccessibility
are ticked and you now click for example theintegrations
category pill will nowaccess control
,accessibility
, andintegrations
be ticked or willaccess control
andaccessibility
be unticked and onlyintegrations
be the sole category being ticked? also the point that applies to the detail and the main page to the same degree, how to signify that you've clicked a category pill and how to revert to the previous category filter selection? yes you have the option to use the back button function in your browser but shouldn't there be at least some signage and/or an option to revert the selection on the page itself as well?and two more aspects about the category pill design in general which are probably out of the scope for this issue. at the moment only the pointer changes on hover but shouldnt the styling be changed on hover and when active aka click, to create an affordance like for other interface components as well? the cursor alone could get missed.
and the background color for the category pills has one general problem, the background color (#E5E5E5) against the card background (#FFFFFF) has a color contrast of 1.3:1. In particular since the pills are clickable, interface components the user is able to interact with have to have a color contrast of at least 3:1 (WCAG 2.2 SC 1.4.11)on the other hand you could also ask a controversial question and question what is the benefit of having the categories on each card? the user is able to control which module cards are shown in the results list by ones selection on the categories filter. problem is there is no switch between AND and OR for filter categories, so it is possible one card could contain only one out of three selected categories, or two or all. but the cognitive load is high to figuring out which categories are selected and which categories a card belong to. but as i said the card is matching the filter criteria and on the details page you have the category pills anyway so are those pills really necessary on a card? and on cards those pills are sort of harming the easy readability and scanability.
- 🇮🇳India Prashant.c Dharamshala
Another point I noticed that could confuse users configuring the settings is the lack of clarification that these settings are for the 'Site Logo' and not a specific logo for navigation.
It would be helpful to add a brief description to each option or a general help text to clarify the purpose of these settings.
Thank you.
- 🇪🇸Spain plopesc Valladolid
Thank you for working on this.
Took a look locally and icon logic works well.
Added some minor comments in the MR.
It would require a review from someone with better knowledge of the HTML structure to confirm that everything is OK.
- 🇮🇳India ehsann_95
ahsannazir → changed the visibility of the branch 3438878-regression-the-drawer to hidden.
- 🇪🇸Spain plopesc Valladolid
Added basic test to ensure that HTML attributes are being placed as expected.
Agree that this might not be enough to test the CSS behavior, but it checks the HTML structure and would avoid unwanted markup regressions.
- 🇨🇦Canada SKAUGHT
Tests for each of the 3 general logo handling options. A bit of a shortcut to setup for op3 to preset the config itself and verify the current fid will be ready for #3436526 and it's tests.
-
chrisfromredfin →
committed 11336f7a on 1.0.x authored by
kwiseman →
Issue #3446414 by kwiseman, divya.sejekan, rkoller, sime: Hovering over...
-
chrisfromredfin →
committed 11336f7a on 1.0.x authored by
kwiseman →
- 🇩🇰Denmark ressa Copenhagen
Another idea could be to allow using Drush, if it's present, since that also offers the
archive:dump
commandBackup your code, files, and database into a single file.
https://www.drush.org/latest/commands/archive_dump/
It could be part of a unified backup sub-module, or in its own sub-module?
- 🇺🇸United States smustgrave
Can issue summary be updated to include proposed solution
- 🇷🇸Serbia finnsky
Problem that:
1. We cannot add test coverage here. Only cover attribute presence. But it is 1% of feature. Probably it is possible to do with Nightwatch. Probably...
2. We already have that type of attributes without any test coverage. Tests shouldn't block it.
3. This is temporary solution until we not have icons library. - 🇺🇸United States smustgrave
So not really sure what can happen here this. Don't think it can move forward without test coverage.
- 🇷🇸Serbia finnsky
@smustgrave,
this is Nightwatch issue: https://www.drupal.org/project/drupal/issues/3393400 ✨ Evaluate adding Nightwatch tests ActiveBut this issue shouldn't be blocked by Nightwatch setup.
We had that type of attributes before.I'm not even sure this feature can be tested by Nightwatch.
Tricky CSS there, until we don't have Library of icons in module. - 🇺🇸United States smustgrave
@finnsky there an issue tracking the nightwatch setup? Should this be postponed on that?
- First commit to issue fork.
- First commit to issue fork.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇪🇸Spain plopesc Valladolid
Thank you for your comments in the MR.
Worked on them and I think this is ready for another round of reviews.
Adding reference to follow-up created: 📌 Add a generic trait for logic to convert references into Urls in LinkWidget Active
Added z-index according to
Update the Z-index of the dropdown, but target something specific to autocomplete vs. the current approach of targeting ui-frontIf an autocomplete dropdown is open, it's reasonably safe to assume it should be above other elements. An autocomplete-specific z-index boost (vs ui-front) is easier to test and less at risk of causing unwanted side effects
.
- First commit to issue fork.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Left some comments/questions on the MR
There's also some existing open threads that I'm not sure have been addressed.
Love this, much cleaner than how we were doing 🙌
- 🇺🇸United States bnjmnm Ann Arbor, MI
Changing the issue title to describe the problem and not a potential solution.
-
chrisfromredfin →
committed bcb89634 on 1.0.x authored by
kwiseman →
Issue #3348954: @todo Add "defer" property to generated <script> tag
-
chrisfromredfin →
committed bcb89634 on 1.0.x authored by
kwiseman →
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
One very small tweak and this will be ready: https://git.drupalcode.org/project/drupal/-/merge_requests/7910#note_317349
- 🇮🇳India narendraR Jaipur, India
I attempted to remove the
@legacy
from the tests, which I added in previous commit, but it seems something was done incorrectly in the MR, resulting in deprecation errors. I would appreciate any assistance. 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.
- 🇳🇿New Zealand quietone New Zealand
@NexusNovaz, thank you for working on this issue! And for you prompt replies to feedback.
I have reviewed the MR and don't think that the example should be split into two parts, one in the constructor and one in the method searchAndSort. Instead, let's use the example of migrate process plugins where the examples are in the Class doc bloc. See MigrationLookup for one example.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇷🇸Serbia finnsky
Let's fix CSS here:
https://www.drupal.org/project/drupal/issues/3450103 ✨ .admin-toolbar__footer CSS fix. [follow up] Active
- 🇷🇸Serbia finnsky
I think that CSS can be simplified. But i don't want to block this feature.
- 🇷🇸Serbia finnsky
1. Imo only Nightwatch tests can cover it. So we need to write test for this after Nightwatch setup will be merged.
2.However, we cannot work on those possible conflicts until one of them is merged
We can work on both in same time.
- 🇪🇸Spain plopesc Valladolid
Thank you!
Tested locally and works as expected with the current icons approach.
I see 2 missing points though:
- We need tests for this
- We need to confirm that this approach is not creating conflicts with
🐛
Special Menu items are rendered as empty links in navigation
RTBC
, which is also affecting to
toolbar-button.html.twig
. However, we cannot work on those possible conflicts until one of them is merged
- 🇮🇳India narendraR Jaipur, India
Moved to NW for fixing deprecation errors in better way
- 🇷🇸Serbia finnsky
I tried to solve this problem differently.
At the moment, the button--has-icon class does not mean that the variable is actually set, all we have is the presence of the --icon variable
I tried to combine
https://css-tricks.com/logical-operations-with-css-variables/
and a double gradient depending on the presence of the variable and everything seems to work well.Please review!
- @finnsky opened merge request.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇦🇺Australia sime Canberra
Rebased. I don't have any design reference to know what this should look like, so I'm just posting what it looks like at the current point.
I think this needs some work:
- Link to
drupal.org →
should be to a permalink rather than an alias, so
/node/3421718
- Link needs an alt text.
- Ideally this would be a popup? But to be consistent with say About text formats on editing pages, it should open in a new window?
- Link to
drupal.org →
should be to a permalink rather than an alias, so
- First commit to issue fork.