Problem/Motivation

Right now there is a single cell table in the update module code that doesn't need to be a table. There is no sorting/select/dragdrop on the this table and therefore doesn't need a table.

Proposed resolution

Replace the table and it's related CSS with a UL list. Try to make the CSS generic enough so it doesn't care for the tags.

Remaining tasks

Create a patch based on the update-version.html.twig from #1898466: update.module - Convert theme_ functions to Twig's result.

User interface changes

N/A

API changes

N/A

Follow-up from #1898466: update.module - Convert theme_ functions to Twig.

Files: 
CommentFileSizeAuthor
#37 Screenshot 2015-05-15 16.57.47.png128.76 KBcasivaagustin
#37 drupal8-update_module-changed-markup-2099293-37.patch2.22 KBcasivaagustin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,410 pass(es). View
#34 drupal8-update_module-changed-markup-2099293-34.patch1.8 KBcasivaagustin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,346 pass(es). View
#31 Screenshot 2015-05-15 14.03.36.png129.4 KBcasivaagustin
#31 drupal8-update_module-changed-markup-2099293-31.patch1.84 KBcasivaagustin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,323 pass(es). View
#15 drupal8-update_module_twig_clean_up-2099293-15.patch26.94 KBRajendar Reddy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal8-update_module_twig_clean_up-2099293-15_0.patch. Unable to apply patch. See the log in the details link for more information. View
#10 update_module_twig_cleanup-2099293-9_and_twig-update_conversion-1898466-88.patch27.17 KBNebel54
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch update_module_twig_cleanup-2099293-9_and_twig-update_conversion-1898466-88.patch. Unable to apply patch. See the log in the details link for more information. View
#4 update_module_twig_cleanup-2099293-4.patch4.16 KBNebel54
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch update_module_twig_cleanup-2099293-4.patch. Unable to apply patch. See the log in the details link for more information. View
#4 update_module_twig_cleanup-screen.png36.61 KBNebel54

Comments

omg-its-maggie’s picture

Could not test UL in template file. Need to have testmodules installed and don't know how.

joelpittet’s picture

Thanks @omg_its_maggie and nice to meet you at the con. Could you post the patch you had? I've updated the issue summary to try to make it more clear as to the goals. If you have any thing else to add to the problem/resolution please do add it to the summary above.

Cheers!

joelpittet’s picture

Issue tags: +dreammarkup
joelpittet’s picture

Issue summary: View changes

Updated issue summary.

Nebel54’s picture

Assigned: omg-its-maggie » Unassigned
Issue summary: View changes
FileSize
36.61 KB
4.16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch update_module_twig_cleanup-2099293-4.patch. Unable to apply patch. See the log in the details link for more information. View

I've updated the html structure of update-version.html.twig to show an UL instead of instead of the single-cell-table.

My patch bases on this patch: https://drupal.org/comment/7938881#comment-7938881 so hopefully the original patch doesn't change too much :)

As testing of this list is a bit tricky at the moment, it's possible to simulate the presence of a new drupal 8 update through adding this array somewhere around line 135 in update.report.inc. If there is any other way of simulating a new drupal version please let me know :)

    $project['releases']['8.0'] = array('version'=>'8.0','version_date' => time(),'date' => date(time()),'version_link'=>'http://url','download_link'=>'http://url','release_link'=>'http://url');
    $project['releases']['8.1'] = $project['releases']['8.0'];
    $project['releases']['8.1']['version'] = '8.1';

    $project['recommended']='8.0';
    $project['latest_version']='8.1';
Nebel54’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: update_module_twig_cleanup-2099293-4.patch, failed testing.

joelpittet’s picture

@Nebel54 that does look better thanks you. Not sure why it's not applying maybe head has moved a bit and needs a re-roll.

Nebel54’s picture

Status: Needs work » Needs review

Actually it would apply if there was a way to tell the testbot to apply the patch from #1898466: update.module - Convert theme_ functions to Twig first… Or should I merge my changes into the other issue?

joelpittet’s picture

@Nebel54 oh yeah forgot this is a followup issue and that one isn't in yet. Since the other is a straight conversion issue i'd try to avoid merging them there, but you can merge them here to see if it will pass. The other one will get in easier if it doesn't have things unrelated to the conversion.

I think we just jumped the gun because at the time this looked like a great template to show what markup cleanup can be done.

Nebel54’s picture

Parent issue: » #1898466: update.module - Convert theme_ functions to Twig
FileSize
27.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch update_module_twig_cleanup-2099293-9_and_twig-update_conversion-1898466-88.patch. Unable to apply patch. See the log in the details link for more information. View

Yes you are right with this "jumped a gun"-thingy. Did not think about it as I am pretty unused to contribue so far :)

I've combined both patches for testing and see if i can help out on the parent issue to get this integrated.

parthipanramesh’s picture

I just tried this. It applies fine but I'm not really sure if this is better. Just my two cents. :)

stackrat25’s picture

Status: Needs review » Needs work

I tried to apply and this was result

git apply update_module_twig_cleanup-2099293-9_and_twig-update_conversion-1898466-88.patch
error: patch failed: core/modules/update/update.report.inc:14
error: core/modules/update/update.report.inc: patch does not apply

stackrat25’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Rajendar Reddy’s picture

Status: Needs work » Needs review
FileSize
26.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal8-update_module_twig_clean_up-2099293-15_0.patch. Unable to apply patch. See the log in the details link for more information. View

re-rolling the patch. Please review.

Gaelan’s picture

It is not clear what needs manual testing here. What needs to be done?

joelpittet’s picture

Status: Needs review » Postponed

For now I'm going to postpone this issue on #1898466: update.module - Convert theme_ functions to Twig It's great to see the possibilities that this could do to make the markup easier to manipulate and change for this listing. If update module's conversion deviates too far from this it may be tricky to apply these changes against it so that's my reasoning.

joelpittet’s picture

#1898466: update.module - Convert theme_ functions to Twig It's finally in! We have a sub issue for #2264833: Convert theme_update_version to update-version.html.twig that is almost in too so we'll wait on that one now.

mgifford’s picture

Status: Postponed » Needs review
Related issues: +#2264833: Convert theme_update_version to update-version.html.twig
SteffenR’s picture

Assigned: Unassigned » SteffenR
Status: Needs review » Active

While looking at the latest DEV the patch provided here is no longer needed, cause the conversion of the page was already done.
Correct me, if i'm wrong - but i think we can set the status to "Reviewed and tested by Community"

SteffenR’s picture

Status: Active » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: drupal8-update_module_twig_clean_up-2099293-15.patch, failed testing.

rpayanm’s picture

Issue tags: +Needs reroll
lanchez’s picture

Assigned: SteffenR » lanchez
lanchez’s picture

Assigned: lanchez » Unassigned
saki007ster’s picture

Seeing the comment from @joelpittet in #18 we need not do anything for this issue. I am not sure why this has been moved to needs re-roll. I think this is done and can be closed.

idebr’s picture

Title: Update Module Twig Clean Up » Replace table with available module updates with better markup

I updated the title to be more in line with the Problem/Motivation. I can confirm the list of available module updates is currently still a table (/admin/modules/update).

Nitesh Sethia’s picture

Assigned: Unassigned » Nitesh Sethia
Issue tags: +#drupalgoa2015

Taking this issue.

Nitesh Sethia’s picture

Assigned: Nitesh Sethia » Unassigned
casivaagustin’s picture

I there, I'm in the DrupalConLA sprint and I want to reroll this issue

casivaagustin’s picture

FileSize
1.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,323 pass(es). View
129.4 KB

I tried to reroll the patches but since these patches are very old, they never woked since all builds have failed, and this functionality have changed in this year for good; I just changed the markup used in the code to use divs instead of table as is requested in the original requirementes.

Also I have updated the css styles and these styles are not taking care about the concrete markup.

Here is an screenshot of the result

Screenshot

casivaagustin’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
lauriii’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/update/css/update.admin.theme.css
    @@ -61,3 +61,7 @@
    \ No newline at end of file
    

    Against Drupal coding standards there should be new line in the end of each file

  2. +++ b/core/modules/update/update.report.inc
    @@ -62,8 +62,8 @@ function template_preprocess_update_report(&$variables) {
    +        '#attributes' => array('class' => array('project-update__update')),
    

    We can use here the short array syntax

casivaagustin’s picture

FileSize
1.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,346 pass(es). View

Thanks for your comments @lauriii here goes another patch with fixes for your comments.

lauriii’s picture

Setting to needs review to wake up our lovely friend testbot :)

lauriii’s picture

Status: Needs work » Needs review

Ups.. :D

casivaagustin’s picture

FileSize
2.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,410 pass(es). View
128.76 KB

I just noted that the module is in the current version is using color-error and painting in red the module, if is in the current version must looks ok so It's has to be green instead of red.

This patch adds that behavior. Screenshot added

Screenshot Green Indicator

maartendeblock’s picture

Status: Needs review » Closed (duplicate)
casivaagustin’s picture