- ๐บ๐ธUnited States smustgrave
This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request โ as a guide.
Tagging. for an issue summary update following the standard template please.
This feature request will require test coverage as well
Thanks.
- ๐บ๐ธUnited States pcate
@smustgrave I've updated the issue summary to follow the standard template.
- ๐บ๐ธUnited States pcate
I've made an initial conversion of @dieuwe #10 patch to a merge request.
- ๐บ๐ธUnited States pcate
It does appear the table style config schema needs to css class option added. The test I added was failing without it. Updated issue summary to reflect this.
- Status changed to Needs review
9 months ago 2:35pm 12 July 2024 - ๐บ๐ธUnited States pcate
Setting to "Needs Review". I added a functionality test to confirm functionality is working, and I also tested manually.
- ๐ฉ๐ชGermany Anybody Porta Westfalica
GREAT work @PCate! RTBC! Removing the solved tags!
- Status changed to RTBC
8 months ago 3:00pm 29 July 2024 - ๐บ๐ธUnited States smustgrave
Nice!
So this is one of those cases test-only feature doesn't work as it reverts the schema change. So applied locally and reverted the changes but kept the schema, so the test will run
Behat\Mink\Exception\ElementNotFoundException: Element matching xpath "//table[contains(concat(" ", @class, " "), " test-css-table-class1 test-css-table-class2 ")]" not found.
Which shows the test itself.
Believe this one is good. Only thing not 100% sure is if it will need a CR.
- ๐บ๐ธUnited States dalemoore
Just testedโworking for me! This just made my day for reals.
Screenshots below.
- Status changed to Needs work
8 months ago 3:40am 8 August 2024 - ๐ณ๐ฟNew Zealand quietone
The issue summary clearly explains the problem and the change.
This is changing the UI, adding tag. The latest before and after screenshots should be available from the issue summary to help reviewers and committers. I updated the IS to state that they are in #26. Testing was done in #25 and manual testing in #26.
I read the MR and have commented on the user interface text.
- Status changed to Needs review
8 months ago 7:06am 8 August 2024 - ๐ฉ๐ชGermany Anybody Porta Westfalica
Thanks @quietone - you're right! Just did that, think you suggestion is good. I also added "Table" to the title to be even clearer about the context. Please review.
- Status changed to RTBC
8 months ago 12:15am 12 August 2024 - ๐ฌ๐งUnited Kingdom longwave UK
Does this need an update hook to add the empty class to any existing views that use the table style?
- ๐ฌ๐งUnited Kingdom catch
#30 is correct, we need a post update for that, otherwise there will be mis-matches when someone next resaves the views UI or a different update runs, marking needs work.
- Status changed to Needs work
5 months ago 7:41pm 23 October 2024 - ๐บ๐ธUnited States pcate
Is there any documentation on how to write config update hooks for views? My understanding is that it is different than regular update hooks?
- ๐บ๐ธUnited States pcate
Using existing views update hooks as examples I tried adding an update hook for the table CSS class schema change. I also added the update hook task to the list of remaining tasks.
- ๐บ๐ธUnited States smustgrave
So with addition of upgrade path will need a test for the that hook.
- ๐บ๐ธUnited States pcate
So with addition of upgrade path will need a test for the that hook.
I've added a db schema upgrade test.
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.
- Status changed to Needs review
3 months ago 1:32pm 23 December 2024 Hi,
Test status -Fail
Steps to reproduce -
Create a view with a table style.
Open the table style options modal in the Views UI
See that there is not a form field to add custom CSS classes to table element.Issue is reproducible. But unable to apply patch. Can see an error when the patch is applied.
Please see the screenshot.- ๐ฎ๐ณIndia sagarmohite0031
Hi,
Test status -Fail
Issue is reproducible.
MR applied successfully but add custom CSS field is still not there.Steps to reproduce -
Create a view with a table style.
Open the table style options modal in the Views UI
See that there is not a form field to add custom CSS classes to table element.Refer Before and after screenshots-
- ๐บ๐ธUnited States pcate
The test failure was with a unrelated core module. Merging in latest 11.x changes included a fix. Tests are now passing.
After apply the MR to Drupal 11 site install, be sure to run database updates (
drush updb
) and clear caches. After applying MR and updates, when you export configuration you should see the schema changes for any views that use a table style. - ๐บ๐ธUnited States pcate
@sagarmohite0031 the new CSS class form field should be at the bottom of the table style options form modal. Your screenshot does not show this.
See @dalemoore screenshot in #26 as an example.
- ๐ฎ๐ณIndia sagarmohite0031
Hello @pcate,
Attaching Before and after updated screenshots-
Please check once - ๐บ๐ธUnited States pcate
@sagarmohite0031 I apologize, the field input actually is under the row class input. It used to be at the bottom of the modal form, but I forgot it was moved.
I manually tested the MR again with both 10.4 and 11.1 and everything still appears to be working, including see the field input.
- ๐บ๐ธUnited States smustgrave
Think one of the last things needed will be a CR. Also before/after screenshots should be added to the summary for quick glance.
- ๐บ๐ธUnited States pcate
I added before/after screenshots to the issue summary and created a draft CR.
- ๐บ๐ธUnited States pcate
Fixed broken images in issue summary. For some reason the did not save previously.
- ๐ฌ๐งUnited Kingdom oily Greater London
Removed 'Needs change record' tag.
I edited the CR as some information was missing. Please refer to the revision log where I have detailed my changes.
Also need to add to it that there is a schema change so users may need to run 'drush updb'.
- ๐บ๐ธUnited States smustgrave
Left 1 comment on the MR. Think adding a small example would be nice as I don't know if the text I provide if the code is auto generating the selector (.) to the front.
Rest of the feedback appears to be addresses so believe this is 98% there.
- ๐บ๐ธUnited States smustgrave
Believe all feedback has been addressed
- Status changed to Needs work
about 1 month ago 9:51am 27 February 2025 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 pcate
I merge in the upstream changes from 11.x and fix the merge conflicts. Setting back to RTBC.
-
longwave โ
committed ea296604 on 11.x
Issue #3045871 by pcate, anybody, dieuwe, dalemoore, smustgrave,...
-
longwave โ
committed ea296604 on 11.x
Automatically closed - issue fixed for 2 weeks with no activity.