Problem/Motivation

The current status report is informative but not highlighting the crucial information really well. A lot of key information gets lost in the rows of green, blue and other color indicators. From a design point of view this page could also be a lot more appealing.

Proposed resolution

We are looking for designers to pick up this page and propose a view alternatives. Yoroy and Bojhan will be available to guide any designers.

Remaining tasks

  • Add default styles for counters and general information #400 to system. Would be nice to get a designers perspective on this.
  • CSS clean-up (BEM conversion / remove class nesting)

User interface changes

The information will be separated in 3 regions:

Error counter at top

3 items with error/warning/checked if there are errors. Link to each item list.
Re-count of items checked if no errors. Link to item list.

General system information table

The aim for this region is to show the basic
info about the development environment.

  • Drupal version
  • Last cron run
  • Button to run cron
  • Web server info
  • PHP info
  • Database info
List of reports
  • Separated lists by Errors, Warnings and Checked.
  • The title for each list should be an anchor for each
    counter in the top regiom.
  • On mobile, thedescription is collapsed by default.

Designs

Status page (no errors)

Status page no errors

Status page (with errors)

Status page with errors

Status page (mobile)

Status page with errors on mobile

Status page in Bartik

Proposal.

API changes

None

Data model changes

None

Files: 
CommentFileSizeAuthor
#487 install-screen-after.png855.6 KBChi
#487 install-screen-before.png1.52 MBChi
#476 redesign_status_report-665790-476.patch75.49 KBchrisrockwell
#476 interdiff-665790-474-476.txt1.34 KBchrisrockwell
#474 redesign_status_report-665790-474.patch75.13 KBvaplas
#473 interdiff-468-473.txt2.79 KBvaplas
#472 system-status-icon-size-mobile-after.png19.28 KBwturrell
#472 system-status-icon-size-mobile-before.png30.12 KBwturrell
#472 system-status-counter-mobile-size-665790-472-interdiff.txt553 byteswturrell
#468 redesign_status_report-665790-468.patch74.89 KBchrisrockwell
#467 interdiff-665790-460-467.txt14.8 KBchrisrockwell
#467 Screen Shot 2017-02-01 at 2.21.01 PM.png52.87 KBchrisrockwell
#467 Screen Shot 2017-02-01 at 2.21.06 PM.png40.67 KBchrisrockwell
#467 Screen Shot 2017-01-31 at 11.16.27 PM.png25.63 KBchrisrockwell
#466 interdiff-460-466.txt4.02 KBleahtard
#466 core-status-page-665790-466.patch72.82 KBleahtard
#461 core-status-page-665790-460.patch72.48 KBchrisrockwell
#459 status-before-after.jpg62 KBleahtard
#459 interdiff-458-459.txt1.67 KBleahtard
#459 core-status-page-665790-459.patch72.48 KBleahtard
#458 core-status-page-665790-458.patch72.14 KBnod_
#458 interdiff.txt1.98 KBnod_
#446 interdiff-437-446.txt1.44 KBvaplas
#446 redesign_the_status-665790-446.patch72.01 KBvaplas
#443 redesign_the_status-665790-437.patch70.56 KBchrisrockwell
#441 redesign_the_status-665790-437.patch70.56 KBchrisrockwell
#438 interdiff-665790-437.txt7.9 KBchrisrockwell
#431 redesign_the_status-665790-430.patch64.46 KBchrisrockwell
#428 Status_report___d8.png58.97 KBchrisrockwell
#427 details-elements.mov1.06 MBtkoleary
#424 contrib-modules-message.png110.36 KBckrina
#422 bartik-error-page-status.png63.57 KBckrina
#420 redesign_the_status-665790-420.patch56.69 KBchrisrockwell
#419 bartik-error-page-statusproposal4.png114.79 KBckrina
#419 bartik-error-page-statusproposal3.png116.37 KBckrina
#419 bartik-error-page-statusproposal2.png116.08 KBckrina
#419 bartik-error-page-statusproposal1.png112.24 KBckrina
#419 status-report-page-665790-419.png118.09 KBckrina
#414 redesign_the_status-665790-414.patch55.58 KBchrisrockwell
#411 redesign_the_status-665790-411.patch51.53 KBchrisrockwell
#405 Screen Shot 2016-12-06 at 8.12.10 AM.png492.74 KBchrisrockwell
#404 c20161206_203252.png67.98 KBdroplet
#401 Status report | drupal 8.2.3 2016-12-06 09-00-06.png198.19 KBGábor Hojtsy
#400 Screen Shot 2016-12-05 at 5.07.20 PM.png122.39 KBchrisrockwell
#399 redesign_the_status-665790-399.patch131.99 KBchrisrockwell
#399 interdiff-665790-399-385.txt7.62 KBchrisrockwell
#390 D5.png12.86 KBdroplet
#390 D3.png63.56 KBdroplet
#386 Screen Shot 2016-12-05 at 8.20.54 AM.png193.38 KBchrisrockwell
#377 redesign_status_report-665790-377.patch140.18 KBchrisrockwell
#377 interdiff-665790-367-377.txt3.32 KBchrisrockwell
#370 redesign-test-665790-370.gif186.62 KBckrina
#367 redesign_the_status-665790-367.patch139.55 KBchrisrockwell
#367 interdiff-665790-364-367.txt2.29 KBchrisrockwell
#365 interdiff-665790-357-364.txt2.09 KBchrisrockwell
#364 redesign_the_status-665790-364.patch68.57 KBpguillard
#361 border-missing.png47.82 KBSumit kumar
#357 interdiff-665790-352-357.txt4.83 KBchrisrockwell
#357 redesign_the_status-665790-357.patch69.09 KBchrisrockwell
#352 interdiff.txt482 byteslauriii
#352 redesign_the_status-665790-352.patch68.91 KBlauriii
#349 interdiff.txt5.89 KBlauriii
#349 redesign_the_status-665790-349.patch68.87 KBlauriii
#347 interdiff.txt1.47 KBlauriii
#347 redesign_the_status-665790-337.patch68 KBlauriii
#345 interdiff.txt19.59 KBlauriii
#345 redesign_the_status-665790-334.patch67.55 KBlauriii
#341 interdiff.txt16.41 KBlauriii
#341 redesign_the_status-665790-341.patch64.99 KBlauriii
#332 interdiff.txt2.25 KBlauriii
#332 redesign_the_status-665790-332.patch60.4 KBlauriii
#329 interdiff.txt393 byteslauriii
#329 redesign_the_status-665790-329.patch59.62 KBlauriii
#327 interdiff.txt61.23 KBlauriii
#327 redesign_the_status-665790-327.patch60.15 KBlauriii
#322 php-info.png68.88 KBckrina
#317 redesign_the_status-665790-317.patch57.94 KBvulcanr
#313 redesign_the_status-665790-313.patch17.59 KBSumit kumar
#313 interdiff.txt1.11 KBSumit kumar
#305 redesign_the_status-665790-305.patch43.04 KBvulcanr
#299 redesign_the_status-665790-299.patch16.99 KBvulcanr
#298 no-tool-bar.png264.92 KBvulcanr
#298 with-toolbar.jpg204.19 KBvulcanr
#298 with-toolbar-1440.jpg300.82 KBvulcanr
#298 no-toolbar-1440.jpg236.33 KBvulcanr
#296 counters.png97.46 KBlauriii
#296 button.png2.94 KBlauriii
#296 gap.png41.08 KBlauriii
#295 d12jj.ply_.st-admin-reports-status(iPhone 5).png80.23 KBvulcanr
#293 interdiff.txt747 bytesManuel Garcia
#293 redesign_the_status-665790-293.patch43.27 KBManuel Garcia
#292 status-page-665790.png118.72 KBckrina
#286 interdiff.txt1.97 KBjoelpittet
#286 redesign_the_status-665790-286.patch43.29 KBjoelpittet
#1 statusreport-after.png111.04 KByoroy
#1 statusreport-before.png133.01 KByoroy
#3 statusreportcleanup-part1.patch1.57 KByoroy
Passed on all environments. View
#12 statusreport2.patch2.24 KByoroy
PASSED: [[SimpleTest]]: [MySQL] 17,528 pass(es). View
#18 statusreport.patch1.91 KByoroy
PASSED: [[SimpleTest]]: [MySQL] 18,523 pass(es). View
#23 Selection_020.png40.14 KBcosmicdreams
#36 drupal8.requirement-ui.36.patch8.14 KBsun
PASSED: [[SimpleTest]]: [MySQL] 35,014 pass(es). View
#37 Table-clean.png65.16 KBBojhan
#38 drupal8.requirement-ui.38.patch9.92 KBsun
PASSED: [[SimpleTest]]: [MySQL] 35,021 pass(es). View
#39 cleaner-status-report-even-more.png74.42 KBBojhan
#43 status-report.png60.03 KBsun
#43 drupal8.requirement-ui.42.patch11.15 KBsun
PASSED: [[SimpleTest]]: [MySQL] 35,006 pass(es). View
#46 status-report-icons.png105.29 KBsun
#46 drupal8.requirement-ui.44.patch12.89 KBsun
PASSED: [[SimpleTest]]: [MySQL] 35,006 pass(es). View
#47 drupal8.requirement-ui.47.patch12.81 KBsun
PASSED: [[SimpleTest]]: [MySQL] 35,015 pass(es). View
#50 drupal8.requirement-ui.42.patch11.15 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 35,053 pass(es). View
#51 first-column-too-narrow.png44.34 KBBojhan
#52 drupal8.requirement-ui.52.patch11.13 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 35,042 pass(es). View
#53 status-report-seven-rtl.png57.83 KBsun
#53 status-report-seven.png56.21 KBsun
#53 status-report-stark-rtl.png75.82 KBsun
#53 status-report-stark.png76.82 KBsun
#53 interdiff.txt1.97 KBsun
#53 drupal8.requirement-ui.53.patch11.34 KBsun
PASSED: [[SimpleTest]]: [MySQL] 36,595 pass(es). View
#65 status-report-d8.png120.08 KBDavid_Rothstein
#65 status-report-d7.png98.06 KBDavid_Rothstein
#74 status-report-665790-74.patch3.12 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 49,427 pass(es). View
#75 status-report-installer.png69.01 KBDavid_Rothstein
#75 status-report-with-problems.png130.52 KBDavid_Rothstein
#75 status-report-without-problems.png132.65 KBDavid_Rothstein
#83 super_obv_status.png78.48 KBtyphonius
#83 more_obvious_pics.png80.8 KBtyphonius
#85 whitebackground.png89.91 KBtyphonius
#88 godaddy-green.png10.73 KBwebkenny
#88 symfony2-green.png18.5 KBwebkenny
#96 665790-status-report-styleguide1.png129.6 KBtompagabor
#109 665790-statusreport-109.patch2.82 KBrootwork
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,869 pass(es). View
#109 interdiff-665790-100-109.txt3.72 KBrootwork
#111 After-Status-report.png149.92 KBmgifford
#111 Before-Status-report.png171.66 KBmgifford
#126 Screen Shot 2016-05-09 at 10.18.37.png5.86 MBBojhan
#126 Screen Shot 2016-05-09 at 10.33.56.png2.79 MBBojhan
#126 Screen Shot 2016-05-09 at 10.37.09.png285.81 KBBojhan
#126 Screen Shot 2016-05-09 at 10.38.32.png400.46 KBBojhan
#154 statusreportcode151.patch3.23 KBSumit kumar
#161 160_status-page_errors_collapsible.png108.8 KBckrina
#189 redesign-status-report_188.patch8.8 KBSumit kumar
#190 665790-190.patch10.41 KBGábor Hojtsy
#190 interdiff.txt10.23 KBGábor Hojtsy
#191 StatusReporyResponsiveIssue.png13.32 KBGábor Hojtsy
#193 redesign_the_status-665790-193.patch10.75 KBlauriii
#193 interdiff.txt15.08 KBlauriii
#198 redesign_the_status-with-new-file665790-197_0.patch11.43 KBSumit kumar
#200 interdiff-193-198.txt4.92 KBjoelpittet
#201 interdiff-193-198-for-real.txt6.16 KBjoelpittet
#202 665790-202.patch11.25 KBjoelpittet
#202 interdiff-198-202.txt14.7 KBjoelpittet
#204 interdiff-202-204.txt25.11 KBjoelpittet
#204 665790-204.patch21.26 KBjoelpittet
#206 665790-206.patch21.85 KBjoelpittet
#206 interdiff-204-206.txt7.39 KBjoelpittet
#207 665790-207.patch24.17 KBjoelpittet
#207 interdiff-206-207.txt5.49 KBjoelpittet
#207 Status_report___desktop.png99.14 KBjoelpittet
#207 Status_report___mobile.png68.33 KBjoelpittet
#208 interdiff-207-208.txt6.68 KBjoelpittet
#208 665790-208.patch26.04 KBjoelpittet
#208 Status_report___desktop.png101.49 KBjoelpittet
#214 status-page__desktop--toggle-left.png119.02 KBckrina
#214 status-page__mb--toggle-left.png76.2 KBckrina
#216 interdiff-665790-208-216.txt15.52 KBchrisrockwell
#216 665790-216.patch37.68 KBchrisrockwell
#216 Screen Shot 2016-09-09 at 9.53.35 AM.png113.19 KBchrisrockwell
#216 Screen Shot 2016-09-09 at 9.53.49 AM.png82.57 KBchrisrockwell
#219 665790-219.patch37.29 KBchrisrockwell
#222 665790-222.patch38.98 KBchrisrockwell
#222 interdiff-665790-219-222.txt5.22 KBchrisrockwell
#224 interdiff-665790-222-224.txt2.22 KBchrisrockwell
#224 665790-224.patch40.59 KBchrisrockwell
#226 interdiff-665790-224-226.txt3.7 KBchrisrockwell
#226 665790-226.patch40.5 KBchrisrockwell
#227 Status_report___tablet.png357.2 KBjoelpittet
#227 Status_report___mobile.png136.26 KBjoelpittet
#229 Desktop_details-element.png154.62 KBchrisrockwell
#229 Mobile_details-element.png88.14 KBchrisrockwell
#230 665790-229.patch41.27 KBchrisrockwell
#230 interdiff-665790-226-229.txt2.96 KBchrisrockwell
#231 665790-231.patch43.66 KBchrisrockwell
#231 665790-interdiff-229-231.txt4.69 KBchrisrockwell
#234 665790-234.patch41.89 KBchrisrockwell
#234 665790-interdiff-231-234.txt4.54 KBchrisrockwell
#237 Status_report-mobile-237.png120.2 KBchrisrockwell
#237 Status_report-desktop-237.png208.55 KBchrisrockwell
#237 665790-237.patch42.35 KBchrisrockwell
#237 interdiff-665790-231-237.txt4.84 KBchrisrockwell
#242 double-cron-button.png91.89 KBlauriii
#242 status-report-rtl.png83.29 KBlauriii
#242 status-report-rtl-info.png74.84 KBlauriii
#246 interdiff-237-245.txt15.25 KBjoelpittet
#246 redesign_the_status-665790-245.patch44.01 KBjoelpittet
#248 interdiff-245-248.txt3.31 KBjoelpittet
#248 redesign_the_status-665790-248.patch44.38 KBjoelpittet
#248 interdiff-245-248.txt3.31 KBjoelpittet
#256 665790-255.patch44.72 KBchrisrockwell
#256 interdiff-665790-248-255.txt15.08 KBchrisrockwell
#261 status-report-details-title-rtl.png39.37 KBlauriii
#261 status-report-vertical-align.png42.48 KBlauriii
#262 665790-262.patch44.9 KBchrisrockwell
#262 interdiff-665790-255-262.txt1.82 KBchrisrockwell
#263 interdiff-665790-262-263.txt6.42 KBchrisrockwell
#263 665790-263.patch45.44 KBchrisrockwell
#275 665790-275.patch16.62 KBSumit kumar
#275 correct-Statu-report-Local host.png136.94 KBSumit kumar
#279 665790-279.patch45.69 KBchrisrockwell
#279 interdiff-264-279.txt640 byteschrisrockwell
#283 665790-283.patch16.62 KBckrina

Comments

yoroy’s picture

FileSize
111.04 KB
133.01 KB

Currently:

First iteration after:

What this does:
- Remove all the green. Don't make the parts that are ok so explicit, it competes too much with the yellow and red parts, which is where you want the attention to focus on.
- Remove the icons for ok as well. again, to focus attention on the parts that do need your attention.
- Brings the explanation texts into the right column, as they expand on the info in the right hand cell. (I assume this was originally done because of the available space, Garland for example has to account for possible blocks in both left and right sidebars. Seven theme doesn't have these)
- If you compare the 'Configuration file' error messages, you'll see that the 'after' screenshot makes it more clear that these are two seperate messages by making each start on a new line.

Not sure yet if the right side cells should be completely white or the light grey.

yoroy’s picture

Oh, and we probably want to remove the use of blue for '.info' (see first row) as well.

yoroy’s picture

Status: Active » Needs work
FileSize
1.57 KB
Passed on all environments. View

Here's a first round, removing colrs form the different stylesheets. I kept selectors that have empty statement blocks in for now, like

table.system-status-report tr.ok th {
}

, because might want to add things back in to some.

Somebody else will have to adjust the php part responsible for the actual html output of the table.

This patch should bring us to this look: http://img.skitch.com/20091222-bykufdjreifpyqju32ecug5ggn.png

kika’s picture

Great initiative, just 2 things:
- it seems gray-and-white zebra row striping and color-coded rows do not mix too well. If light yellow and pink try to convey a message, gray and white row color feels totally random. Why "database updates" and "file system" are on white, "access", "cron", "gd" on gray? Can we make an exception and disable zebra-striping here?
- I can not wait for fresh icon replacements here, drupal 4.5 is still haunting us ;)

yoroy’s picture

- Good point on the zebra thing. Yes, we'll have to remove it for this table or override it through CSS
- Icons: have you seen #606490: Drupal GPL icons - a softfacade initiative ?

wretched sinner - saved by grace’s picture

What are the thoughts on leaving in the green ticks for items that are ok? I agree that it could be a bit more distracting, but nothing quite beats looking down the list to see all green ticks and to know everything is running well...

dww’s picture

Component: update.module » base system

- Generally, I'm in favor. This is similar to the direction we recently took the d.o project node UI...

- The status report is handled by system.module, and populated by every module that implements hook_requirements(). This isn't specific to update.module at all. Moving to the "base system" component (not sure, maybe system.module would be more appropriate?).

- I'm drawn to wretched sinner's position in #7. I had a hard time letting go of the icons *and* colors as we worked on the project UI. It's nice to have *some* indication that "everything's cool", especially on a page like the status report. Yeah, it doesn't have to SCREAM that to you in every possible way, but I agree it's nice to have something to explicitly mark that a given row is happy, instead of just having a neutral white background and expecting users to either read all the text to know it's okay, or just tune it out. If we expect them to tune out all text in a white background, that seems to set a dangerous precedent. In that case, why display it at all? :/

yoroy’s picture

Thanks dww, component wise, I'm always just guessing.
I always start with the most rigorous dress-down, leaves me with small change further down the road ;-)
Will return to this after xmas etc.

yoroy’s picture

Status: Needs work » Needs review

needs review now that testbot is back…

yoroy’s picture

FileSize
2.24 KB
PASSED: [[SimpleTest]]: [MySQL] 17,528 pass(es). View

Next round…

Exploration in gray, weighting the warning levels a bit against eachother. I don't mind showing checkmarks, but they should speak very softly:
http://skitch.com/yoroy/np2cm/status-report-d7

Current CSS-only patch makes it go like this:
http://skitch.com/yoroy/np2cq/status-report-d7

Dries’s picture

Issue tags: +Favorite-of-Dries

Tagging this beauty.

mrfelton’s picture

Can I ask why we are using light yellow and pink 'pastel shades' whilst normal site error messages are displayed in a vivid orange/red/green? Shouldn't we try and be consistent with the rest of the site? I find that these light shades blend in with the surrounding grey too much and it all just looks a little wishy-washy, like we are scared to alert the user to these problems so instead of plainly telling them they have a problem with a bright orange, we gently break it to them with a washed out yellow.

yoroy’s picture

Status: Needs review » Needs work

Have to make sure that any changes to how this table gets rendered should apply to other instances as well, like here:

Requirements problem | Drupal

yoroy’s picture

mrfelton: haven't looked at the colors at all, yet. Thanks for pointing it out.

yoroy’s picture

Status: Needs work » Needs review

#12: statusreport2.patch queued for re-testing.

yoroy’s picture

FileSize
1.91 KB
PASSED: [[SimpleTest]]: [MySQL] 18,523 pass(es). View

Previous patch did not apply anymore

moshe weitzman’s picture

so, we need a developer to do the table rendering changes?

yoroy’s picture

Yes please.

marcvangend’s picture

Marked #753764: Duplicate and incorrect error messages during install as duplicate.

In that issue, my point (besides another that has already been fixed) was: The status report and the system messages should be more consistent in their use of color. I think the consistent use of the same colors (for example: http://drupal.org/files/issues/system-status-colors-admin.png) will create a better UX. Also, when an error message is shown on the same page as the status report (screenshot: http://drupal.org/files/issues/d7-install-error_1.png) the combination of colors is just plain ugly.

On a sidenote, I'm a little surprised that #538666: Seven theme not right with the status report screen hasn't been mentioned here before. Although the focus over there is a little more on accessibility, it's basically a duplicate of this one.

cosmicdreams’s picture

FileSize
40.14 KB

This patch needs a little more work. see how the formatting is inconsistent? Some left-hand table cells are "open" and others are almost closed. See screenshot.

cosmicdreams’s picture

Status: Needs review » Needs work
yoroy’s picture

Correct. I don't know how to handle the rendering of rows that have descriptions (see #3).

cosmicdreams’s picture

When dealing with the table markup the answer might be similar to what I found in #746678: Markup issue with tables (admin/appearance/update)

cosmicdreams’s picture

Also, I think someone might have brought this up before, the yellow used for warnings appears to light in IE on my screen. I have to look very closely to see a difference between the light yellow and the white on my screen. The end result is very displeasing to my eyes.

mgifford’s picture

Ok, so I'm looking at this issue & wanting to look for places where this messaging color background is being implemented. In talking to Bojhan & yoroy on IRC it seems important to look for places where the pattern is being implemented:

It's being implemented here - admin/reports/dblog
with - modules/dblog/dblog.css

It's being implemented here - admin/reports/dblog
with - modules/dblog/dblog.css

Status Report - admin/reports/status
with - modules/system/admin.css

Available Updates -
with - modules/update/update.css

And the install.php
with - ?? I'm not sure how to call up and compare those tables..

In Seven's error messages
with - themes/seven/style.css

It's also in Bartik & Garland I think:
themes/bartik/css/style.css
themes/garland/style.css

Are there others that are missing?

mgifford’s picture

I added a patch here that touches on this issue http://drupal.org/node/639368#comment-3211194

markabur’s picture

Also see #906738: Status report need identifying icons (WCAG 2.0), which does some cleanup by removing the icon on "OK" rows.

yoroy’s picture

Version: 7.x-dev » 8.x-dev

Badump.

Bojhan’s picture

Status: Needs work » Needs review
Issue tags: -Needs design review
sun’s picture

Status: Needs review » Needs work
Issue tags: +Needs design review

This totally makes sense.

It does not make sense to show everything in green on the Status report page. The positive state on the Status report is informational only.

So the Status report output should use the .info state instead of the .ok state for positive assertions (met requirements).

And that leads to the subsequent issue: Using a blue color for neutral information (.info state) is inane. The .info state should just be grey (or even more simple, use the theme's default table row background - but yet, provide a dedicated CSS class so that themes are able to target that special table row case, if they want).

However, implementation-wise, I'm not OK with removing the .ok CSS classes that visualize a green/good state. There are use-cases for visualizing an OK/good state; e.g., to highlight a positive state after performing an action.

I'm also a bit on the fence, whether this should be applied to the installer. In the installer's case, the green .ok state is actually used in the context of a confirmation (as mentioned above). As a user installing Drupal, it's good to see which requirements are positive and which are negative. So I'd say that requirements rows on Status report != Installer. I'm not sure whether we use the same code for both, but if we do, then this would boil down to something like this per table row:

  // Use .ok in the installer to confirm requirements;
  // otherwise .info for neutral exposure on Status report.
  'attributes' => array('class' => defined(MAINTENANCE_MODE) ? 'ok' : 'info'),
sun’s picture

Status: Needs work » Needs review
Issue tags: -Needs design review
FileSize
8.14 KB
PASSED: [[SimpleTest]]: [MySQL] 35,014 pass(es). View

Attached patch implements just that.

Bojhan’s picture

FileSize
65.16 KB

Whoa, interesting - I actually had my status report open and refreshing it with this patch applied created so much more calmness.

I agree that using the blue color for neutral information is insane, we have a special signal for it being "nothing". I always thought white was neutral :). I do agree that we need to be cautious just adding this to the installer, the green is a good emotional way of signifying that they are almost there. I am open to discussing it more, because removing the green also makes it more of a "see this rainbow of things you did good and didn't".

I do think we should try to use the gray color for the left column. The scanability of this table is determined by your ability to match label on the far left in the left column to the description in the right column. By using color you create proximity, this allows the eye to use the left gray bar as a "landmark" to move down from - visually each item becomes more a "chuck" than by using no color.

sun’s picture

FileSize
9.92 KB
PASSED: [[SimpleTest]]: [MySQL] 35,021 pass(es). View

Re-introduced the visualization of the vertical (instead of horizontal) table header column.

Bojhan’s picture

Assigned: Unassigned » yoroy
FileSize
74.42 KB

Yoroy can you give this a review?

cleaner-status-report-even-more.png

marcvangend’s picture

This looks pretty good.

The only thing I'm not sure about, is the way the descriptions (e.g. under "Node Access Permissions" in Bojhan's screenshot) are displayed. IMHO the descriptions break the vertical rhythm, make the lay-out look broken and take up to much space compared to their importance. Especially the description under "Node Access Permissions" reads like a help text rather than details of a status report. Can we think of a way to make them less intrusive?

yoroy’s picture

Thanks for this. Overall a much cleaner look with clearer focus on what needs your attention. I agree with the long descriptions breaking the flow. I figured this approach would make it easier to accomodate these descriptions but doing it like this drives unneccessary attention to the rows with description, just because they have one. So that was not such a good idea after all :)

We can also look into shortening the descriptions, happy to look into that.

Bojhan’s picture

sun’s picture

FileSize
60.03 KB
11.15 KB
PASSED: [[SimpleTest]]: [MySQL] 35,006 pass(es). View

The actual descriptions shouldn't be touched in this issue; instead, see #1463956: Shorten and clean up status report messages.

To resolve the vertical rhythm issue (which I agree with), I'd propose to remove the colspan over the entire table width, and instead, put the description in the "right" value column only.

I.e., this: :)

status-report.png

yoroy’s picture

Very nice. If bot approves and code gets another lookover its rtbc imo. Design-wise this is ready to go.

marcvangend’s picture

Yes, thank you, much better!

sun’s picture

FileSize
105.29 KB
12.89 KB
PASSED: [[SimpleTest]]: [MySQL] 35,006 pass(es). View

Attached patch additionally fixes the vertical alignment of the icons (by removing the separate icon column and sneaking a new .icon style into core through this patch :-D)

status-report-icons.png

sun’s picture

FileSize
12.81 KB
PASSED: [[SimpleTest]]: [MySQL] 35,015 pass(es). View

Additionally resolved that "todo" ;) by simply switching the icon markup from DIV to SPAN.

markabur’s picture

The line spacing in those descriptions has become really tight.

aspilicious’s picture

Status: Needs review » Needs work

I rly rly love this patch but it's back to needs work

  1. .icon needs rtl styling for margin-right
  2. shortcut and toolbar styling is broken
aspilicious’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
11.15 KB
PASSED: [[SimpleTest]]: [MySQL] 35,053 pass(es). View

It's dangerous to touch .icon in here it is used in other places around core.
After discussion with Sun we agreed to use the pathc in #43.

I tested that one (also in RTL), and it is RTBC.
Reuploading to ensure the right patch gets pushed.

Bojhan’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
44.34 KB

Chrome on Windows 7 and the first column is too narrow, I am not enough of a CSS guru to figure out what it is.
first-column-too-narrow.png

aspilicious’s picture

Status: Needs work » Needs review
FileSize
11.13 KB
PASSED: [[SimpleTest]]: [MySQL] 35,042 pass(es). View
sun’s picture

Assigned: Unassigned » sun
FileSize
57.83 KB
56.21 KB
75.82 KB
76.82 KB
1.97 KB
11.34 KB
PASSED: [[SimpleTest]]: [MySQL] 36,595 pass(es). View

Re-rolled against HEAD. (Also note that this code lives in the platform-statusreport-665790-sun branch now)

Attaching a full stack of screenshots, including RTL output.

This looks ready to fly for me.

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

Let it fly then!

Tor Arne Thune’s picture

Definitely an improvement.

marcvangend’s picture

Re #53: What happened to the toolbar position in status-report-seven.png? Is it safe to assume it has nothing to do with this patch?

sun’s picture

yes, that's not toolbar but admin_menu_toolbar, and I apparently did not have the position:fixed option enabled and wasn't at the top of the page when doing that screenshot ;)

marcvangend’s picture

OK. No further questions, your honor. :-)

sun’s picture

Issue tags: +API change

Briefly discussed this with @jacine and created a follow-up issue for #1591744: Clean up CSS and markup for status report

This change should land regardless of that, as it's a definite improvement over now.

Furthermore, the removal of the default severity REQUIREMENTS_INFO/_OK from hook_requirements() should be announced and documented as a change notice.

cosmicdreams’s picture

Looks great, +1 for RTBC

catch’s picture

Title: A cleaner look for the status report » Change notification for: A cleaner look for the status report
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Committed/pushed to 8.x.

Leaving open for the change notice.

yoroy’s picture

Thanks for seeing this through folks, glad to see this made it in.

sun’s picture

Title: Change notification for: A cleaner look for the status report » A cleaner look for the status report
Priority: Critical » Normal
Status: Active » Fixed

Created the change notice: http://drupal.org/node/1600936

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

David_Rothstein’s picture

Status: Closed (fixed) » Needs review
FileSize
120.08 KB
98.06 KB

This issue was a great improvement in the case where your status report actually contains a warning or error.

But what about the case where everything is OK? That was barely mentioned in the comments above, and none of the screenshots either.

It seems like it might be a regression to me so I thought I'd post some comparison screenshots for comment.

In Drupal 7, your eyes scan down the list and see everything is green, which is a quick visual indication that everything with your site is OK (actually would be better in this case if the checkboxes were on each row too, but those seem to have disappeared at some point in the D7 dev cycle):

status-report-d7.png

In Drupal 8, everything is white, and there is no visual indication that your site is OK. If you're not familiar with how this page works, you'd basically have to read the entire page to know that things are going well with your site:

status-report-d8.png

So, I'm wondering if we should consider improving the "everything is OK" case. Possibilities include:

  1. A single visual indicator somewhere near the top of the page (i.e., a single green checkbox/message saying that everything is OK). Or,
  2. Each row becomes green and has a green checkbox next to it (but only in the "everything is OK" case - as soon as one row is not OK, all the OK ones lose their green color and checkbox and go back to white so that the visual focus is entirely on the things that are broken).

Any thoughts?

markabur’s picture

I agree -- "All OK" is a critical message to convey, but the status screen leaves you to infer that for yourself. We give clear feedback when you do the wrong thing but then go silent when you get it right.

Looking at the D8 screen above it's impossible to tell if any of the settings ought to change. Is the file system *supposed* to be writable? Is the "Not enabled" status for Upload progress something that needs fixing?

With that in mind, I like the idea of #2 better. A single message at the top of the screen is easy to miss, especially on small screens, and would leave me less confident about particular line items such as "Not enabled" for Upload Progress being OK as-is.

sun’s picture

I don't think that coloring everything in green is appropriate, because as you can see in the screenshots in #65, there are status report rows that are actually not fully OK (e.g., the second to last "Upload progress" row).

If really absolutely needed, then I'd rather opt for injecting a first additional row into the table that says:

"Overall status: (OK|Warnings|Errors)"

However, I don't think we need any kind of positive visual confirmation here in the first place. The committed patch explicitly kept the positive confirmation for the Requirements pages in the installer and update.php, because in both cases, we actually want to communicate all table rows as individual steps that have to be successful. The Status report page is not involved in a user task that checks whether requirements are met; it's a pure report, which you are free to ignore.

In case the concerns are based around the discovery of warnings and errors in a potentially larger report page, then IMHO the proper answer to that would be to sort the table rows before output, so errors come first, warnings second, and everything else after.

aspilicious’s picture

I think this looks damn good now. I don't think there would be many users panicking because they don't see any green. Users "panic" if they see red or yellow.

Yes we could add a new line that says "OK, warnings, errors". But what colour do you give that message when there are actually errors...

Bojhan’s picture

I am not sure either, you have to get used to the idea that - if nothing is indicated, all is good. However I think @David is correct in stating, that might not be obvious. I am on the fence about this one, lets see what more feedback brings us :)

mrfelton’s picture

My 2c.. I think that green clearly indicates all is good, red clearly indicates there is an issue, and no color doesn't really give anything away. The page looks nice white (I personally prefer the look), but it also looks ok when all items are green. But I do think using green for each item to indicate that everything is ok is actually more useful to users, and it also makes them feel good in the knowledge that their Drupal setup has all checklist items passing.

Bojhan’s picture

@mrfelton So, the reason we removed it - and you can get that from the information in the discussion above is that by making everything "green" you introduce this "rainbow of color" this is hard to explain, but essentially creates an effect where because you are using color so extensively to communicate meaning the real important messages like errors/warnings are very hard to scan for, and if they occur the page feels visually very heavy/overwhelming. So I am not inclined to agree, with going back to the original state - because its exactly that sense of overwhelming we want to remove from this page so people actually end up looking at it.

mrfelton’s picture

Yes, I sort of agree with that, when you have a mixture of errors and passes the green and red clash and look pretty horrible together (not sure if I agree that the really important messages like errors/warnings are very hard to scan though - it's still bloody obvious that red items have a problem, and green ones do not). What about making all the items show as green if everything passes, and if there are one or more errors leave everything white with just the errors being highlighted in red (I think thats what David_Rothstein was suggesting in #65.2). When there are no erros on the page, that needs to be communicated to people somehow.

markabur’s picture

if nothing is indicated, all is good

At the very least, the text at the top should say this.

David_Rothstein’s picture

FileSize
3.12 KB
PASSED: [[SimpleTest]]: [MySQL] 49,427 pass(es). View

What about making all the items show as green if everything passes, and if there are one or more errors leave everything white with just the errors being highlighted in red (I think thats what David_Rothstein was suggesting in #65.2).

Yup, exactly. The attached patch implements that, so we can see how it behaves and are all on the same page.

I don't think that coloring everything in green is appropriate, because as you can see in the screenshots in #65, there are status report rows that are actually not fully OK (e.g., the second to last "Upload progress" row).

I don't think it actually makes sense to have "informational" messages as a separate category from "OK" messages. Either something is OK or not, and if it's not OK, it should be a warning. (If we want to additionally have a visual indicator for "here is how to make your site even better" that could be a separate indicator, but it doesn't change the fact that it's OK as is and should be fine to color it green. "OK" just implies good enough; it does not imply 100% perfectly ideal :)

So in short, I'd like to see REQUIREMENT_INFO go away entirely, since it would simplify both the code and concepts here. But that would get into some other problems, so I'm not proposing it as part of this patch.

In the particular case of "Upload progress", I think the problem there is that it's worded almost like a warning but then isn't one? Probably the module needs to decide which it is, and change the wording accordingly.

David_Rothstein’s picture

Screenshots of the above patch. The only thing that is different from current core is the first screenshot (the "everything is OK" case). The others are unchanged from what's currently in Drupal 8.

1. Status report when everything is OK (the green rows and checkboxes help you quickly scan the page and understand your site is doing fine):

status-report-without-problems.png

2. Status report when everything is not OK (the green colors and checkboxes disappear to help you focus on what's wrong):

status-report-with-problems.png

3. Installation requirements page (the green colors are there to indicate your progress, but the green checkboxes are not, to avoid conflicting too much with the "error" icon in the row that does indicate a problem):

status-report-installer.png

sun’s picture

1) The OK icons in the first screenshot don't make any sense to me. I also don't understand why the entire page is in green, because I did not perform any action that would need confirmation.

2) The sudden disappearance of the formerly green rows confuses me. I guess something must be wrong with them, so I better check all of the text information to be sure, and after doing so, I still won't understand why they are no longer green.

3) In the 3rd screenshot (the install/update requirements pages), I expected the OK icons from the 1st screenshot, since this is an actual task/check list.

4) The patch removes support for info status entirely, which I strongly disagree with.

yoroy’s picture

Ugh. This basically brings us back to the situation for which this issue was opened in the first place :-(

I can understand the need to have a signal somewhere that everything is ok. Plastering the green back all over the page is not the way to do it. Signal the anomalies, not the default state.

David_Rothstein’s picture

OK, it sounds like people would prefer the solution in #65.1. Any thoughts on what that should look like exactly? I'm having trouble picturing it.

It's almost like a standard Drupal status message (something green with words in it that gets displayed near the top of the page) but I'm not sure a message is really the right concept here.

Bojhan’s picture

I would imagine its a column on the top of the table "Overall status" or something silly like that.

klonos’s picture

...if nothing is indicated, all is good

Nope, ...if nothing is indicated, you need to assume that all is good. If we needed the ok/green out of the way, then we'd better hide the ok entries completely out of view. But this then would convert this from a "status" page to a "problems" page, which is what I see one side of people suggesting here.

I've been monitoring this issue here almost since the day it got filed and I'd like to say that I really like how currently (in D7) everything is green when everything is ok. I never heard anyone complain about too much green "overwhelming" them in check lists and this is a check list (a check list in order to have your Drupal site running as smoothly as possible). On the contrary, I think that green gives people the sense of accomplishment and that it communicates that everything is running smoothly. This is a status page and we need to see everything that might matter and their respective status clearly indicated. This to me (generally as an IT admin - not only within the Drupal context) translates to red for problems, yellow/orange for warnings, gray/blue for info and green for ok.

To this end, I see the following entries as info:

- Drupal version
- DB system/version
- GD library version
- PHP version
- Web server version

Now (with the exception of GD lib) I cannot imagine a situation where any of these would be shown as either red or yellow. If that were the case, than something would be seriously wrong with the server and the site wouldn't work. So, I wouldn't be able to see the status page in the first place. I'd consider these entries purely info, have them shown at the top of the status page and group them in a neutral color (gray/blue). I believe that this would reduce the chances of having lots of various colors shown "randomly", "all over the place" and thus would prevent most of the confusion or the overwhelming feeling some complain about. So, I need to ask: is there any serious usability case that dictated that all the entries in the status page are listed alphabetically instead of being grouped together in "logic" groups?

BTW, if it were up to me, I'd even go as far as merging the "Drupal" version entry with the "Drupal core update status" one. Perhaps only then having this row in green/yellow/red would make sense.

David_Rothstein’s picture

BTW, if it were up to me, I'd even go as far as merging the "Drupal" version entry with the "Drupal core update status" one. Perhaps only then having this row in green/yellow/red would make sense.

Slightly off-topic, but yes; see #309040-10: Add hook_requirements_alter() (I think it would require that issue).

sun’s picture

Assigned: sun » Unassigned
Status: Needs review » Active

Let's do #67 / #79 then; i.e., simply a first table row that says:

"Overall status: (OK|Warnings|Errors)"

and which is colored accordingly (green|yellow|red).

REQUIREMENTS_INFO severities/rows should be ignored for the overall status condition/calculation; i.e., if there are no warnings and no errors, then the overall status is OK/green.

Furthermore, the additional row should only be generated and output on the Status report page. Not in the requirements check of the installer or update.php.

[Speaking of, this is one of the tables we want to convert to the new render element #type 'table', and doing so might be the only way to inject the Overall status row on the Status report page only [not verified].)

typhonius’s picture

This issue reminded me of the output received when running tests. (x failed, y warnings, z pass) kind of thing.

My instant reaction when greeted with an all white/light grey status report is that I am unsure about whether everything is alright. To some extent the ticks down the left hand side or the "status ok" message at the top already suggested, or a message similar to a simpletest report would make things more instantly that everything is ok at a glance.

Or to put it in other words - the page needs more green.

The other thing I've noticed with the lack of green filling up the table, is the lack of contrast available for the red and yellow messages. If the green background is gone (which I fully agree with) then as #14 states, the remaining colours should be a lot stronger to stand out more.

I've mocked up a couple of really rough examples.

The first uses the same border drupal_set_message gives to error and warning messages.
more_obvious_pics.png

The second uses the border colour for the background colour.
super_obv_status.png

The colours can of course be changed so think not of how terrible/clashing they are, but rather of how much more they stand out. The status report now says "YOU HAVE AN ERROR" rather than "you may have an error but it's not going to jump out at you"

My disclaimer that I'm not a designer likely does not need mentioning.

sun’s picture

@typhonius: IMHO the actual bug there is that Bartik uses a (dark) grey background color for all tables, which inherently deemphasizes the contrast/visibility of colored rows. I don't know why Bartik is doing that. Perhaps it's just because Drupal core contained default styles for all tables since ~2004 or so. I removed those default styles in #1813792: Remove ugly default CSS styles for table only very recently. If you'd ask me, then we should remove that background color for tables in Bartik, too. That's a separate issue though.

typhonius’s picture

FileSize
89.91 KB

@sun: I agree the background in the bartik table does dilute the colour. Removing from bartik wouldn't be a bad idea at all.

Checking white background for the right side of the table:
whitebackground.png

I can't decide if the colours are still washed out or vivid enough to be seen; I think I've been staring at it too long.

tsvenson’s picture

Late to the game, but the Status report page just created an itch for me. Glad to see the progress being made on beautification. However I have two questions:

  1. Does it really have to be in alphabetic order?
  2. Can't we add a button at the top "Hide all OK items"?

To answer my own questions.

1, Yes, it would be an UX improvement if we instead of alphabetic order of all items can create logical groups of items. For example:

  • Drupal Status - Core, etc
  • Maintenance/security - Database schema updated, Cron status, Module/Theme status, When Backup last was run (Backup and Migrate), etc
  • Third Party integration - Google Analytics, XML-sitemap, etc
  • Third party libraries - The stuff in /sites/all/libraries
  • Cahce info and status
  • Web server features - PHP info, file system, GD library, Upload progress
  • System server components - the stuff Drupal runs on (OS, database, file system, etc)

These groups needs to be static, that is - modules can't invent new groups like is done using the "package" keyword for modules.

Also, when possible there need to be info/link to the feature/module that the report item belongs to for added readability.

It should also become good practice that modules that for example adds third party integration add a report item here. Disqus is one such integration that currently does not report here. Then Media YouTube/Flicker/Vimeo etc are other good candidates to.

The items in the groups can be alphabetically listed, that OK.

2, a big switch at the top that simply hides all "green" items except the Drupal core group at the top.

Could also be complemented with a drop-down jump-list with the group names. Useful as the list will get longer on many sites as more projects does the right thing and adds a reporter here.

I think these improvements, together with those already discussed in this thread will make this page both easier to read and more useful at the same time.

tsvenson’s picture

Oh, and a third question:

3, How come there is no option for alerting admin about when something covered on the status report needs attention?

Currently only the version check on Drupal core and modules/themes does that here.

But that is probably better to discuss in a new issue?!

webkenny’s picture

FileSize
10.73 KB
18.5 KB

This is really a great issue because the design of the Status Report was always a "needs work", in my mind. With that said, I don't think we need to reinvent the wheel here but I do think we are going against practiced axioms of computer science by removing any indication that things are "Ok" . Green is the universal indicator on all levels that something is "Good" - Your light on your very first PC Tower, your Macbook power adapter, most other electronic devices in the world. Let me clarify that:

  • I do agree that an all green status report indicating each message is of the same type doesn't make a lot of sense.
  • Not so sure I agree that no indicator of "No problems found" is a good thing.

On the command line, and I'm borrowing from Symfony2 only because it's handy - if I run a check on my install I get something like this:

 OK       PHP version must be at least 5.3.3 (5.3.14 installed)
 OK       PHP version must not be 5.3.16 as Symfony won't work properly with it
 OK       Vendor libraries must be installed
 OK       app/cache/ directory must be writable
 OK       app/logs/ directory must be writable

Even when downloading packages via your favorite package managers, you get some indication of all clear via [OK] when, for example, compiling.

Now on the web UI side... again Symfony2 only because it's handy... does something like this:

GoDaddy (and other DNS providers) also provide some kind of green all clear:

I guess we should start gravitating toward some of these accepted practices and not away; Also, I'm not sure why we hate the color green. ;)

I don't think having a ton of green checks is useful but I do think a "No problems found" on the Status report at the very top would make a user a very happy camper. Don't make me think about what is in each row if I don't care. Just tell me it's fine and let me get back to my content.

The very words, "Status Report" means you're looking for a "status" -You want to know if your status is green, red, or yellow like you're used to hearing in your job every day. (Boss: "Jones! What is the status report on that project?!" Jones: "Green, boss!")

What sun proposes in #82 makes the most sense to me. Maybe not even a row. Maybe nice little flag from behind the table that is colored appropriately and reads the adjective we want to give it.

typhonius’s picture

drupal_set_message could be used here to give an overall status of the system. If there are ANY errors the status is error. If there are no errors but there are warnings then the status should be warning. If there are no errors or warnings then the status can be status/green!

Perhaps the entire status table should be hidden if it's all green with an option for the user to expand and view should they require.

I initially didn't like what tsvenson said in #86 about only showing non-green items by default but the thought has grown on me. There will be absolutely no mistaking what is causing the issue then.

webkenny’s picture

Me and my flags. +1 to drupal_set_message() :)

sun’s picture

A dsm() doesn't work for me, since the process that generates the status report page could trigger status/error messages on its own, which in turn could easily lead to conflicting error messages in the messages area; i.e., one saying error/warning, and another saying "All is OK my friend." (which would be a lie in that case ;))

webkenny’s picture

Good point, sun. So perhaps my flag behind the table would be a workable solution after all. I was thinking something like a little square div on the right hand top of the table. Something that would say either [OK - Checkmark] or [Warnings: 5 Found] or [Errors: 2 Found] depending on the state. This way, we only draw the user to what they need to see.

I should have some time today to roll a patch for my crazy idea.

markabur’s picture

the process that generates the status report page could trigger status/error messages on its own

Couldn't the status report include that sort of message as part of the report, in a non-confusing way? ("Additionally, there was 1 error while generating the status report: [...]"). If I am interested in the status of the site, then I am also interested in the fact that the status report generation process produces errors -- so this sort of message really ought to be a formal part of the report if possible.

typhonius’s picture

Perhaps indeed the very first line of the status report is a 'system summary' which is coloured green/yellow/red depending on whether there are errors/warnings etc.

This could remain always visible with errors/warnings also visible underneath. Should the user wish to see the full report they may click something to unhide the remaining status rows.

LewisNyman’s picture

Issue tags: +styleguide

We should align this design with the visual consistencies introduced in the new style guide: #1986434: New visual style for Seven

tompagabor’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
129.6 KB
3.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,799 pass(es). View

Now its use the new style guide.

longwave’s picture

Status: Needs work » Needs review

Failed due to unrelated segmentation fault, which is being tracked down in #2226351: Segmentation fault in Drupal\image\Tests\ImageStylesPathAndUrlTest

96: 665790-statusreport-styleguide-96.patch queued for re-testing.

LewisNyman’s picture

+++ b/core/modules/system/css/system.module.css
@@ -308,7 +308,16 @@ tr .ajax-progress-throbber .throbber {
+/**
+ * Hide elements visually, but keep them available for screen-readers.
+ * ¶
+ * This class load background, and other formats, just the text is hidden
+ * visually.
+ */
+.visually-hidden-text {
+  overflow: hidden;
+  text-indent: -999em;
+}

There is some whitespace here and I'm a bit apprehensive about adding a new class, can we move it below the .invisible class and rename it to hide-text? Just so it's not easily confused with visually-hidden.

tompagabor’s picture

FileSize
2.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 665790-statusreport-100.patch. Unable to apply patch. See the log in the details link for more information. View

Change class to hide-text as recommended.

LewisNyman’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/css/system.admin.css
    @@ -188,19 +188,13 @@ a.module-link-configure {
     table.system-status-report td {
    -  padding: 6px;
    -  vertical-align: top;
     }
     table.system-status-report td:nth-child(-n+2) {
    -  background-color: rgba(0, 0, 0, 0.04);
     }
    ...
     [dir="rtl"] table.system-status-report td.status-icon {
    -  padding-left: 0;
    -  padding-right: 6px;
     }
    
    +++ b/core/themes/seven/style.css
    @@ -694,17 +691,17 @@ table.system-status-report tr:last-child {
    -table.system-status-report tr.ok {
    +table.system-status-report tr.ok:not(:hover) {
       color: #255b1e;
    -  background-color: #e5ffe2;
    +  background-color: transparent;
     }
    -table.system-status-report tr.warning {
    +table.system-status-report tr.warning:not(:hover) {
       color: #840;
    -  background-color: #fffce5;
    +  background-color: transparent;
     }
    

    We now have some empty selectors, we can just delete them now.

  2. +++ b/core/modules/system/css/system.admin.css
    @@ -188,19 +188,13 @@ a.module-link-configure {
     table.system-status-report td {
    -  padding: 6px;
    -  vertical-align: top;
     }
     table.system-status-report td:nth-child(-n+2) {
    -  background-color: rgba(0, 0, 0, 0.04);
     }
    ...
     [dir="rtl"] table.system-status-report td.status-icon {
    -  padding-left: 0;
    -  padding-right: 6px;
     }
    

    We not have some empty selectors, we can delete these as well.

  3. +++ b/core/themes/seven/style.css
    @@ -694,17 +691,17 @@ table.system-status-report tr:last-child {
    -table.system-status-report tr.ok {
    +table.system-status-report tr.ok:not(:hover) {
       color: #255b1e;
    -  background-color: #e5ffe2;
    +  background-color: transparent;
     }
    -table.system-status-report tr.warning {
    +table.system-status-report tr.warning:not(:hover) {
       color: #840;
    -  background-color: #fffce5;
    +  background-color: transparent;
     }
    

    Do we need to set the background to transparent? Aren't they always transparent when there is no hover?
    I think we can get rid of the text colour as well. Which means can just delete all of this CSS?

sun’s picture

Status: Needs work » Active

@tompagabor et al: Can you move that Seven-specific restyling patch into a new issue, please? This issue has actually seen a commit (a very long time ago) already and was only re-opened, because some concerns were raised later on.

We should actually move that follow-up discussion on exposing an overall status indicator into a new issue, too.

LewisNyman’s picture

Status: Active » Needs review
rootwork’s picture

Is this really still active then?

Bojhan’s picture

Well yhea.

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Ok, then it needs a re-roll + improvements as per #101.

Status: Needs work » Needs review
rootwork’s picture

FileSize
2.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,869 pass(es). View
3.72 KB

Done.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
149.92 KB
171.66 KB
realityloop’s picture

Issue tags: +Amsterdam2014

Before:

After:

Bojhan’s picture

Can this pattern be documented?

Bojhan’s picture

Status: Needs work » Closed (won't fix)

#2227401: Apply the seven style guide to the status report Fixing everything in this issue. I thought the other one just did Seven, but I see it did more.

mgifford’s picture

Sorry Bojhan, thought this had already gone through you.

rootwork’s picture

Yeah that's why I asked ;) Ah well.

yoroy’s picture

Status: Closed (won't fix) » Closed (duplicate)

ehm :)

David_Rothstein’s picture

Title: A cleaner look for the status report » A cleaner look for the status report - followup to provide a clear visual indication at the top of the page when everything is OK
Status: Closed (duplicate) » Needs work
Issue tags: +Needs issue summary update

Fixing title and status. The recent patches here were for something else, but there was a commit here in #61 and then the issue was reopened again in #65 to reconsider that design.

In the "your site is all OK" case, Drupal 8 is definitely still missing a clear indication to the site administrator of that fact, compared to Drupal 7.

This could be moved to a separate issue, but there is a lot of good discussion about that followup above so it's probably better to just leave it here.

There seems to be a fair amount of consensus here for a solution similar to #82 - a row at the top of the table, or some other visual indicator at the top of the page, which provides the administrator with a clear visual indication of the overall site status (OK/Warnings/Errors).

longwave’s picture

Version: 8.0.x-dev » 8.1.x-dev

This will go into 8.1.x at the earliest.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Bojhan’s picture

Title: A cleaner look for the status report - followup to provide a clear visual indication at the top of the page when everything is OK » Redesign the status report page
Bojhan’s picture

Issue summary: View changes

Updated the issue summary. While this is cleaner - its not really appealing. I've added some examples of interesting status report pages.

catch’s picture

@Bojhan it doesn't look like the examples made it in the issue summary update.

Bojhan’s picture

yoroy’s picture

Compare the above with the current state:

Only local images are allowed.

yoroy’s picture

Issue tags: +visual design
mgifford’s picture

Those are so nice! Thanks @Bojhan & @yoroy would be great to see those in place in some form!

ckrina’s picture

I prepared some fast proposals to decide what information we need/want before jumping to the visual design.

1. Quick summary with a counter for errors, warning and things that are already ok. It can be also helpful a link to the detailed list of errors/warnings/already done things (2b). In the case that there are no errors, but we still have some warnings, I would maintain the 3 counters. In the case that everything is fine, my proposal is to hide the counter component and show only a message saying that everything is ok. Do you agree with hiding the counter? And, do you agree on maintaining the 3 counters even 1 ore them is 0?

2. Summary with general system information like Drupal version, Database version, etc. Only information that is just informative. A good way to decide what information we need here could be: add only the minimum information we need to paste into an issue to explain what is our development environment. What kind of information do you think it would be interesting to have here?

3. I would suggest also a separated list for errors, warnings and things that are already ok. On small devices, we could show only the name, and open/hide it with a toggle because there are some items that have a lot of text. Do you agree on that?

Status page with errors
Status page without errors
Status page in small devices

rootwork’s picture

These look fantastic, thanks @ckrina!

Do you agree with hiding the counter? And, do you agree on maintaining the 3 counters even 1 ore them is 0?

Personally, yes, I agree with hiding the counter if all are 0, and I agree on showing all three if just one or two is 0.

What kind of information do you think it would be interesting to have here? [summary]

I took a look at the current 8.2.x status page. Here are the items I think we should at a minimum include in this summary:

  • Drupal version
  • Web server type and version (one line, as it is now)
  • Database type and version (could be on one line or two, as it is in your mockup)
  • PHP version and memory (could be on one line or two) with link to more info
  • Last cron status and link to run cron

I think the PHP extensions and PHP OPcode caching, which are either enabled or disabled, could be moved to your third section of notifications, unless it seems confusing to divorce it from the line about PHP in the summary, in which case that could live in the more info link.

I would suggest also a separated list for errors, warnings and things that are already ok. On small devices, we could show only the name, and open/hide it with a toggle because there are some items that have a lot of text. Do you agree on that?

I definitely agree on only showing the name, with an open/close toggle on small devices.

I basically agree on separate lists for errors, warnings, and a third list. But the third list is a bit more complicated than "things that are already OK." The third list really includes both:

  • things that are OK (if they weren't OK, they would show up as errors)
  • informational items — things like the image toolkit, the unicode library, and the search index progress; these have no "error state" to them

Contrib modules also add lots of informational items too. So I'm not sure if it makes sense to actually have four lists (I think probably not) or just be aware that we're actually jumbling two different types of things together (as we do already, of course).

But overall, I really really like this!

yoroy’s picture

Status: Needs work » Needs review
Issue tags: +ux-hierarchy

I had a chance to review this at frontend united with @ckrina. There's some more details to flesh out but, I'm really happy with this direction. Thanks for working on this @ckrina!

Thanks @rootwork for constructive feedback. Could you list some examples of what contrib adds to this page?

rootwork’s picture

@yoroy Here's a few examples of contrib modules in 8.2.x:

  • Google Analytics: Displays warning before it's been set up; informational notice once it is.
  • Token: Out of the box, the most recent D8 version displays the following informational notice: "Token types are not defined but have tokens: $info['types']['entity']"
  • Search API: Out of the box and just after installing, it displays the following warning: "The default Drupal Search module is still enabled. If you are using Search API, you probably want to uninstall the Search module for performance reasons."
  • CAPTCHA: If logging CAPTCHA blockings, displays the following informational notice: "Already blocked [NUM] form submissions"

So there's a few examples from roughly the top 30 modules for D8. I can collect more if that would be helpful.

yoroy’s picture

Thanks! That's good enough for now :)

ckrina’s picture

FileSize
102.87 KB

Here are some first visual proposals for the status page.
For now, the difference between them is on the "General system information". The idea is to find out if adding icons looks like a good improvement.

ckrina’s picture

FileSize
100.82 KB
58.57 KB
63.36 KB
rootwork’s picture

I think the icons are great! And if we didn't use icons, I'd prefer the two-section layout for general system info (option 2) rather than one-section (option 1) or a separate section for each item (option 3). I think the two-section layout has the right balance for scanning.

But really, I think the icons would be helpful and also make a sometimes-scary page more friendly.

The other parts all seem good, but I again just want to highlight the third section, marked "Correct" — this would be both things that are "correct" and things that are just informational. I wonder if we could come up with another label for that section.

ckrina’s picture

FileSize
107.12 KB
118.14 KB
63.86 KB

Thank you for the feedback @rootwork! I also prefer the icons version mainly for the same reason: it makes the information more “digestible”.
I also changed the “correct” label for “checked”. Anyway, it’s completely open to suggestions.

I’ve also updated 2 proposals for the quick summary with some visual changes. One of them tries to add a visual style closer to the current Seven style guide.

Error page 04

The other one is to discuss the idea of having some graphics to enrich the information and make it more friendly. Going for it, I would say that we are using a “graphic” as icon in the “status” menu item in the toolbar. The problem with that is we are complicating the development.

Error page 04

Which one do you prefer? Graphics or not?

I've also updated a version of the status page without errors.

Error page 04

rootwork’s picture

Looking great!

nit: The cron section should also have the link to run cron, in my opinion (i.e. don't bury that link in the more info)

I'm of two minds on the circular graphics. On the one hand, I can see how some people would interpret that as looking more "friendly." On the other, to me it just seems like superfluous noise -- the sizes of the items within the circle are only dependent on the count of notices on the status page, which doesn't really represent anything. In other words, a 100% "full" circle doesn't mean a whole lot, and a 10% "full" circle when there are 25 notices means something different than a 10% "full" circle when there are only 10 notices. And, of course, not all errors are equal in severity.

The graphics also push down the page a bit more. Aside: What does this page look like in mobile? Stacked up? If so then these graphics would really push things down.

So I guess I can be convinced, but they don't jump out at me as necessary.

On the everything's-fine status page, can we use the same graphic and checkmark as the others -- whether that ends up being the first boxed version or the percentage-circle version -- rather than the standard Drupal success message? At least in my opinion that would be better -- a big checkmark saying everything is good :)

Bojhan’s picture

We've discussed this at the UX team hour, and reached consensus that this is a good direction. Visually splitting information; separating basic information, highlighting the different statuses and counts, and ability to go to them.

For this to reach completion and ready for patching, we think that we need to explore a few more ideas:
- Find what could fit in the 6th box :)
- Explore placing the basic information at the top, and the "warning", "error", "checked" below this.
- Explore using one table, instead of a separated table.
- How do we deal with error states on the Drupal version, last cron run, etc.
- Design the mobile version of this page.

Gábor Hojtsy’s picture

Issue tags: +DevDaysMilan
ckrina’s picture

Here is a version using one table instead dividing by king of message (error, warning, others). To show the differences between them I’ve used the same background color that is used today for errors and warnings.
I’ve also uploaded a version without title in the table, and also without title in the Basic info section.

Status page not separated table

Status page not separated table, no titles

Here is a version of the status page without error or warnings as @rootwork suggested (thanks for the feedback!), changing the regular message with another one styled like the ones in the group error, warning + checked recap region.

Status page without errors

Here, I’ve also added a “run cron” primary button to fit the 6th box. I would prefer to use another information instead of using a button because it’s not the same kind of information.

I’ve uploaded this other version with several explorations:
- One of the items in the Basic info section in red when there is an error related to it. I added the red background and changed the icon. It clearly shows that there is an error, but if we don’t add it there it might be confusing. And adding all the information there would break completely the design. Maybe we could convert it to a link to the error?

Other explorations

Here are also 2 proposals for the mobile version. The difference between them are:
- In one of them the icons in the Basic info section are hidden. It can help placing more information in less space.
- The table not speared in Errors/warnings, checked messages. I think it's better this way in mobile.
- The table does not have the title. I helps saving space also.
Status page mobile verions

Sumit kumar’s picture

@prabhu9484 told me about this issue. The design is cool @ckrina, we need to change the html structure of status report page according to this design. Is this fine ? Request @Gábor Hojtsy @Yoroy, @Bojhan to confirm.

yoroy’s picture

Confirmed that yes, this will be a new implementation of this page and we can change the HTML to make this design possible.

Sumit kumar’s picture

@yoroy we have requirements array in which i have got length of array 25. So we need to print that array value one by one because the order of that value are not according to design. i need your confirmation to do this?
If You want to suggest any thing else then i will try that too.

Sumit kumar’s picture

So i have submitted a patch for status report as per the status-page-665790-142-04.png. I could find all field except "checked". What does this field signify? @prabhu9484 will join the call today to clarify the patch.

yoroy’s picture

FileSize
245.06 KB

Looks like only the html output was changed and no styling was added:

prabhu9484’s picture

yes - we created the HTML structure - wanted to confirm if it is ok before proceeding with the styling
we could not understand "checked" field - we could not find it in the array

Gábor Hojtsy’s picture

Copied from slack for reference:

@prabhu.s: so the status report has rows currently and they may be warnings, errors, etc.
@prabhu.s: the things that are not issues (not warnings, errors, etc) SOME of them are “ok” but some of them are just informational
@prabhu.s: eg. PHP 5.6.2 is information that the system checked not necessarily ok or not :slightly_smiling_face:
@prabhu.s: it is just a check that was done and did not produce errors
webchick’s picture

Issue tags: +Needs markup review

We discussed this issue/patch in UX meeting today. Here's what I remember (all comments in relation to #142):

- Because it's the more frequently changing information, start with the error/warning/status summary, followed by the Drupal/database/PHP stuff.
- For mobile view, the version on the right with headings seem better, since there's something to link to from above.
- However, the "Details" links to jump down should actually be links; else they look like text descriptions just like those in the table below.
- We also discussed whether there was a need to colour the details row backgrounds and/or the headings, and decided there was not.
- The mock to change Drupal icon to a red, flaming error is actually great, but probably needs more discussion since this involves logic and we might want to have a head-to-head about what constitutes a "warning" vs. "error" for cron, for example.

On the patch itself (#146), I didn't initially understand why the change was being made from tables to divs/spans; but it was explained that this is because we want the responsive tables in mobile. I'm not actually familiar enough with the markup standards there to confirm this one way or another, so tagging for markup review.

There is a lot of copy/paste code in here, which was stated that the reason we need that is because the elements are being done out of order. 19, then 24, then 3, etc. Those numeric indexes also greatly reduce the readability of the code. I would suggest instead building the array you actually want in system_status_preprocess(), like $system['drupal'], $system['db'], etc. and then using foreach() loops to cut down on the boilerplate. This also helps prevent a scenario where 19 is database on one site but 19 is PHP on another because of different modules being installed.

Sumit kumar’s picture

Status: Needs work » Needs review
FileSize
3.11 KB
Sumit kumar’s picture

Sumit kumar’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

+++ b/core/themes/stable/templates/admin/status-report.html.twig
@@ -15,25 +15,57 @@
+      {% if key == 0 or key == 3 or key == 5 or key == 14 or key == 24 %}
...
+      {% if key != 0 and key != 3 and key != 5 and key != 14 and key != 24 %}

How do you tell that these keys mean something special? Did you try adding a few contributed modules and seeing if indexes 14 and 24 still display what you think they do?

prabhu9484’s picture

Thanks Gabor - as discussed inthe UX call yesterday, can you post an example code snippet we can refer to ?

Gábor Hojtsy’s picture

So if you look at where this template is used, in SystemInfoController:

  public function status() {
    $requirements = $this->systemManager->listRequirements();
    return array('#theme' => 'status_report', '#requirements' => $requirements);
  }

which points to SystemManager:

  public function listRequirements() {
    // Load .install files
    include_once DRUPAL_ROOT . '/core/includes/install.inc';
    drupal_load_updates();

    // Check run-time requirements and status information.
    $requirements = $this->moduleHandler->invokeAll('requirements', array('runtime'));
    usort($requirements, function($a, $b) {
      if (!isset($a['weight'])) {
        if (!isset($b['weight'])) {
          return strcasecmp($a['title'], $b['title']);
        }
        return -$b['weight'];
      }
      return isset($b['weight']) ? $a['weight'] - $b['weight'] : $a['weight'];
    });

    return $requirements;
  }

So what this does is it lists the requirements results by weight. Which means their numbers can be anything. But the data is collected from hook_requirements() (if you look at ModuleHandler's invokeAll()). That is defined as:

/**
 * Check installation requirements and do status reporting.
 *
 * This hook has three closely related uses, determined by the $phase argument:
 * - Checking installation requirements ($phase == 'install').
 * - Checking update requirements ($phase == 'update').
 * - Status reporting ($phase == 'runtime').
 *
 * Note that this hook, like all others dealing with installation and updates,
 * must reside in a module_name.install file, or it will not properly abort
 * the installation of the module if a critical requirement is missing.
 *
 * During the 'install' phase, modules can for example assert that
 * library or server versions are available or sufficient.
 * Note that the installation of a module can happen during installation of
 * Drupal itself (by install.php) with an installation profile or later by hand.
 * As a consequence, install-time requirements must be checked without access
 * to the full Drupal API, because it is not available during install.php.
 * If a requirement has a severity of REQUIREMENT_ERROR, install.php will abort
 * or at least the module will not install.
 * Other severity levels have no effect on the installation.
 * Module dependencies do not belong to these installation requirements,
 * but should be defined in the module's .info.yml file.
 *
 * During installation (when $phase == 'install'), if you need to load a class
 * from your module, you'll need to include the class file directly.
 *
 * The 'runtime' phase is not limited to pure installation requirements
 * but can also be used for more general status information like maintenance
 * tasks and security issues.
 * The returned 'requirements' will be listed on the status report in the
 * administration section, with indication of the severity level.
 * Moreover, any requirement with a severity of REQUIREMENT_ERROR severity will
 * result in a notice on the administration configuration page.
 *
 * @param $phase
 *   The phase in which requirements are checked:
 *   - install: The module is being installed.
 *   - update: The module is enabled and update.php is run.
 *   - runtime: The runtime requirements are being checked and shown on the
 *     status report page.
 *
 * @return
 *   An associative array where the keys are arbitrary but must be unique (it
 *   is suggested to use the module short name as a prefix) and the values are
 *   themselves associative arrays with the following elements:
 *   - title: The name of the requirement.
 *   - value: The current value (e.g., version, time, level, etc). During
 *     install phase, this should only be used for version numbers, do not set
 *     it if not applicable.
 *   - description: The description of the requirement/status.
 *   - severity: The requirement's result/severity level, one of:
 *     - REQUIREMENT_INFO: For info only.
 *     - REQUIREMENT_OK: The requirement is satisfied.
 *     - REQUIREMENT_WARNING: The requirement failed with a warning.
 *     - REQUIREMENT_ERROR: The requirement failed with an error.
 */
function hook_requirements($phase) { ... }

So there is no key to identify the important parts. You cannot tell if an item needs to be pulled into that table based on its description, value or title. I would introduce a new key here for those elements. Something like "highlight priority" => 4 where 4 is the number of the slot from the special table (assuming we numbered the slots sequentially). Or "highlight" => "database" where we name each slot based on what we want to display there and the names would correspond to the cell names. Maybe the second one is nicer.

Then the values from the SystemInfoController status() method are converted further for display in the theme preprocess hook in system.admin.inc's template_preprocess_status_report(). That function would turn the elements with "highlight" keys to its own array or individual elements with those names, eg. "database" to display in the right table cells.

That way the source tells us which target highlight the information goes to in the standard data format extended with a new key while the preprocess function places it in convenient variables for the theme to display.

I would approach it that way. Does that make sense?

Sumit kumar’s picture

Thanks @Gábor Hojtsy sound like a good plan, I have some time to look into this will let you know.

ckrina’s picture

Here are the designs after the decisions made on june 28 UX meeting:

Error re-count at top.

  • 3 items with error/warning/checked if there are errors. Link to each item list.
  • Re-count of items checked if no errors. Link to item list. Should we add a clearer message saying “No errors found”?

General system information table with 6 pieces:

  • Drupal version
  • Last cron run
  • Button to run cron
  • Web server info
  • PHP info
  • Database info

List of all checked items ordered by errors/warnings/checked.

  • Separated each of them by a title, that could also act as #anchor for the “Details” link on the re-count top region.
  • On mobile I would collapse the description to fit the table on narrower devices.
  • Collapsible groups. I proposed the collapsible behavior, collapsed by default on mobile (to have a shorter and less overwhelming page) and opened in desktop. But having everything more “ordered” means hidden in this case, so maybe more difficult to find. I remember we talked about that on the meeting but I don't remember we decided anything. What do you think about making the groups collapsible?
    • Pros:
      • Shorter and digestible page.
      • User can hide the info he doesn’t need.
      • Useful in small displays.
    • Cons:
      • Hidden info
      • Collapsed element with another collapsed item inside (item description).

No collapsible:

No errors

Status page no errors

With errors

Status page with errors

Mobile

Status page mobile

Collapsible:

With errors

Status page with errors collapsible

Mobile

Status page mobile collapsible

ckrina’s picture

FileSize
5.85 KB

I attach also the svg for the General system information table. Sumit kumar: do you still need the Illustrator files?

Sumit kumar’s picture

thanks @ckrina the design is fantastic. Yes it would we nice if u send illustrator files.

Gábor Hojtsy’s picture

@ckrina: I think the updated designs look great. Reviewing now since we did not have time to review in the Ux meeting. Only two comments:

  1. The double collapsible stuff looks confusing especially on mobile. I *think* the meeting discussion was leaning more towards not having a collapsible group. I think that is even more apparent in light of the collapsible individual rows on mobile.
  2. There is no button to run cron on mobile. I think that is an oversight and it was not intended to be hidden? Otherwise I am not sure there is a way on a phone to do the cron run?

@Sumit: any progress with the patch? Do you need help?

Sumit kumar’s picture

Hi @Gábor Hojtsy so i have implement the new design for counter of error, warning and checked and also so list of found error, warning and checkd in attached png (for more reference see the new-design-implementation.png ). So only one part is left and that is Genral system information. So i am stuck with this ,so i need your help to resolve this.

For counter and listing for found error here is the code

<table class="system-status-report">
  <tbody>
    {% set counter = requirements %}
    {% set error = 0 %}
    {% set warning = 0 %}
    {% set checked = 0 %}
    {# <pre> {{ dump(foo) }} </pre>  #}
    {% for i in 1..counter|length %}  
      {% if counter[i].severity_status == 'error' %}
        {% set error = error + 1 %}
      {% endif %}
      {% if counter[i].severity_status == 'warning' %}
        {% set warning = warning + 1 %}
      {% endif %}
      {% if counter[i].severity_status == 'info' %}
        {% set checked = checked + 1 %}
      {% endif %}
    {% endfor %}
    {% if error != 0 or warning != 0 or checked != 0 %}
      <span class="all-exits system-status-report-counter">
        <span class=" system-status-report__status-title all-same system-status-report__status-icon system-status-report__status-icon--error"> </span>
        <span class='error er-wr-detail'> {{ error }} {{'error'|t }}</span>
      </span>
      <span class="all-exits system-status-report-counter">
        <span class="system-status-report__status-title all-same  system-status-report__status-icon system-status-report__status-icon--warning"></span>
        <span class='warning er-wr-detail' >{{ warning }} {{ 'warning'|t }}</span>
      </span>
      <span class="all-exits system-status-report-counter">
        <span class="system-status-report__status-title all-same system-status-report__status-icon system-status-report__status-icon--checked"></span>
        <span class="checked er-wr-detail">{{ checked }} {{ 'checked'|t }} </span>
      </span>
    {% elseif error != 0 or checked != 0  %}
      <span class=" half-width system-status-report__status-title system-status-report__status-icon system-status-report__status-icon--error"> {{ error }} {{'error'|t }} </span>
      <span class=" half-width system-status-report__status-title system-status-report__status-icon system-status-report__status-icon--checked">{{ checked }} {{ 'checked'|t }} </span>
    {% elseif warning != 0 or checked != 0 %}
      <span class=" half-width system-status-report__status-title system-status-report__status-icon system-status-report__status-icon--warning">{{ warning }} {{ 'warning'|t }}</span>
      <span class="system-status-report__status-title system-status-report__status-icon system-status-report__status-icon--checked">{{ checked }} {{ 'checked'|t }} </span>
    {% else %}
      <span class="full-width test system-status-report__status-title system-status-report__status-icon system-status-report__status-icon--checked">{{ checked }} {{ 'checked'|t }} </span>
    {% endif %}
    <tr class="system-status-report__infomation-title system-status-report__error-found"> <td> {{ 'Errors found'|t }}</td></tr>
    {% for requirement in requirements %}
      {% if requirement.severity_status in ['error'] %}
        <tr class="system-status-report__entry system-status-report__entry--{{ requirement.severity_status }} color-{{ requirement.severity_status }}">
          <th class="system-status-report__status-title system-status-report__status-icon system-status-report__status-icon--{{ requirement.severity_status }}">
            <span class="visually-hidden">{{ requirement.severity_title }}</span>
            {{ requirement.title }}
          </th>
          <td>
            {{ requirement.value }}
            {% if requirement.description %}
              <div class="description">{{ requirement.description }}</div>
            {% endif %}
          </td>
        </tr>
      {% endif %}
    {% endfor %}
    <tr class="system-status-report__infomation-title system-status-report__warning-found"> <td>{{ 'Warnings  found'|t }}</td></tr>
    {% for requirement in requirements %}
      {% if requirement.severity_status in ['warning'] %}
        <tr class="system-status-report__entry system-status-report__entry--{{ requirement.severity_status }} color-{{ requirement.severity_status }}">
          <th class="system-status-report__status-title system-status-report__status-icon system-status-report__status-icon--{{ requirement.severity_status }}">
            <span class="visually-hidden">{{ requirement.severity_title }}</span>
              {{ requirement.title }}
          </th>
          <td>
            {{ requirement.value }}
            {% if requirement.description %}
              <div class="description">{{ requirement.description }}</div>
            {% endif %}
          </td>
        </tr>
      {% endif %}
    {% endfor %} 
    <tr class="system-status-report__infomation-title system-status-report__checked-found"><td>{{ 'Checked'|t }}</td></tr>
    {% for requirement in requirements %}
      {% if requirement.severity_status in ['info'] %}
        <tr class="system-status-report__entry system-status-report__entry--{{ requirement.severity_status }} color-{{ requirement.severity_status }}">
          <th class="system-status-report__status-title">
            {{ requirement.title }}
          </th>
          <td>
            {{ requirement.value }}
            {% if requirement.description %}
              <div class="description">{{ requirement.description }}</div>
            {% endif %}
          </td>
        </tr>
      {% endif %}
    {% endfor %}
  </tbody>
</table>

So i have write this code to print counter and listing of founded error, warning or checked.
@Gábor Hojtsy Please let me correct if it is wrong way or give me direction.

Sumit kumar’s picture

FileSize
186.23 KB

@Gábor Hojtsy this is the png for referance.

Gábor Hojtsy’s picture

Issue tags: +sprint

Putting on usability sprint. Will respond to Sumit later, sorry, not yet.

Gábor Hojtsy’s picture

@Sumit: 2 key points of feedback:

  1. Logic like the counting of errors, warnings, etc. should happen in template_preprocess_status_report() in system.admin.inc, not in the template. That allows other templates in themes to depend on that logic without lots of copy-pasting. Set up new theme variables there based on the counts.
  2. All text that maybe singular and plural should use translation for both variants. See http://hojtsy.hu/blog/2013-jul-24/drupal-8-multilingual-tidbits-10-conte... for examples on translating singular/plural with {trans} in twig.

As for how to proceed with the general information section, I provided guidance on that above. I would update hook_requirements() with a new key to place concrete status report items in specific categories and then use again the preprocess function (template_preprocess_status_report() in system.admin.inc) to put these into variables for the theme for easy use.

Given that you seem to be looking at a nicely themed colorful page already, if you could fix these, then it would all be down to markup and CSS review I think where I would need to pass on the helper flag to someone else more knowledgable/opinionated in that area.

Sumit kumar’s picture

Thanks @Gober for your feed back.

Gábor Hojtsy’s picture

@Sumit: are you still continuing working on this? There is about a week to get this in for 8.2 and it would probably need to go through some review rounds :)

Sumit kumar’s picture

Yes @Gobor i am working on counter part of this task.

ckrina’s picture

Gábor thanks for the feedback!

I've updated the designs with #150 @webchicks' annotation, about the decision we made regarding the row backgrounds and I forgot.

- We also discussed whether there was a need to colour the details row backgrounds and/or the headings, and decided there was not.

I've also updated the summary. Feel free to change/correct anything.

ckrina’s picture

Issue summary: View changes
FileSize
617.76 KB

Also @sumit here is the Illustrator source file.

ckrina’s picture

Issue summary: View changes

Fixed summary images. Sorry for adding more comments. :-)

Sumit kumar’s picture

Status: Needs work » Needs review
FileSize
1007 bytes
1007 bytes
Gábor Hojtsy’s picture

  1. +++ b/core/modules/system/system.admin.inc
    @@ -163,6 +163,24 @@ function template_preprocess_status_report(&$variables) {
    +    $counter = $requirement;
    +    $max = sizeof($counter);
    +    $error = 0; ¶
    +    $warning = 0; ¶
    +    $checked = 0; ¶
    +    for($i = 0; $i <= $max; $++) {  ¶
    

    You can just foreach() on the requirements I think.

  2. +++ b/core/modules/system/system.admin.inc
    @@ -163,6 +163,24 @@ function template_preprocess_status_report(&$variables) {
    +        if ($counter[$i]['severity_status'] == 'error' ) {
    +          $error = $error + 1;   ¶
    +        }
    +        if ($counter[$i]['severity_status'] == 'warning' ) {
    +          $error = $warning + 1;   ¶
    +        }
    +        if ($counter[$i]['severity_status'] == 'info' ) {
    +          $error = $checked + 1;   ¶
    +        }
    

    $variables[] would need the new keys for error, warning, etc. added. Here you are incrementing the same number also for all the cases, which is not good.

Also I think you had CSS as well which you did not yet post. Would be nice to get all the twig, PHP and CSS and see it together as a patch. So they all can be reviewed while you work out the PHP.

Sumit kumar’s picture

Status: Needs work » Needs review
FileSize
6.86 KB

added patch with css

  • catch committed 99a456c on 8.3.x
    Issue #665790 by sun, yoroy, aspilicious: A cleaner look for the status...

  • catch committed 99a456c on 8.3.x
    Issue #665790 by sun, yoroy, aspilicious: A cleaner look for the status...
hedrickbt’s picture

I am at DrupalCorn sprint looking at this. I see notes from @catch about a commit, but I didn't see the changes until I applied the patch.

Here's how I cloned
git clone git clone --branch 8.3.x https://git.drupal.org/project/drupal.git

Here's how I patched
patch -p1 < ./redesign-status-report_173.patch

YesCT’s picture

FileSize
93.03 KB

@hedrickbt #179 (180) seem to be an artifact of an old commit from May 2012 (#61)

So the most recent work since then (for example patch in #178) has not been committed. :)

I investigated by asking around and clicking on the commit hash (180) :)

hedrickbt’s picture

+++ b/core/themes/stable/templates/admin/status-report.html.twig
@@ -16,6 +16,29 @@
+   {% if error != 0 or warning != 0 or checked != 0 %}
...
+    {% elseif error != 0 or checked != 0  %}
...
+    {% elseif warning != 0 or checked != 0 %}
...
+    {% else %}

Caught up on reading the ticket and it looks like the last patch is current with suggestions around the 3 status items.

I am reviewing core/themes/stable/templates/admin/status-report.html.twig. I am trying to understand the if conditions and I am confused about how we can ever get past the 3 or condition initial if... when the other conditions that are on the ...else side would also be met by the same criteria.

Maybe the or conditions need to be changed to and. I need to test each of the individual combinations to make sure they are working as expected.

I am hoping to get back to this some time later in the week.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Getting there in term of logic AFAIS. I think the counting happens at the right place now. Lots of cosmetic things to fix though to make it easier to review for markup and CSS (two of which I am not proficient in, so cannot help with). The cosmetic fixes to do though are:

  1. +++ b/core/modules/system/system.admin.inc
    @@ -163,6 +163,22 @@ function template_preprocess_status_report(&$variables) {
    +  ¶
    +  $variables['error'] = 0;
    +  $variables['warning'] = 0;
    +  $variables['checked'] = 0;
    +   ¶
    +    foreach( $variables['requirements'] as $i => $requirement) {  ¶
    +        if ($variables['requirements'][$i]['severity_status'] == 'error' ) {
    +          $variables['error'] = $variables['error'] + 1;   ¶
    +        }
    +        if ($variables['requirements'][$i]['severity_status'] == 'warning' ) {
    +          $variables['warning'] = $variables['warning'] + 1;  ¶
    +        }
    +        if ($variables['requirements'][$i]['severity_status'] == 'info' ) {
    +          $variables['checked'] = $variables['checked'] + 1;   ¶
    +        }
    +    }
    

    Please watch your code style, several line ending spaces are here, as well as indentation with 4 spaces instead of 2 all around the foreach.

  2. +++ b/core/themes/stable/css/system/system.admin.css
    @@ -6,7 +6,7 @@
    -.layout-container {
    + .layout-container {
    
    @@ -48,7 +48,7 @@
    -.panel {
    + .panel {
    
    @@ -59,14 +59,14 @@
    -.compact-link {
    + .compact-link {
    ...
    -small .admin-link:before {
    + small .admin-link:before {
    
    @@ -76,7 +76,7 @@ small .admin-link:after {
    -.system-modules thead > tr {
    + .system-modules thead > tr {
    
    @@ -210,7 +210,7 @@ small .admin-link:after {
    -    padding: 10px 40px 10px 6px;
    +  padding: 10px 40px 10px 6px;
    

    ... and several others. These are not required changes and just make the patch harder to review.

  3. +++ b/core/themes/stable/css/system/system.admin.css
    @@ -387,3 +390,38 @@ small .admin-link:after {
    +}
    \ No newline at end of file
    

    Please add a newline at the end of the file.

  4. +++ b/core/themes/stable/templates/admin/status-report.html.twig
    @@ -16,6 +16,29 @@
     <table class="system-status-report">
    +
    +   {% if error != 0 or warning != 0 or checked != 0 %}
    +      <span class="all-exits system-status-report-counter">
    +        <span class=" system-status-report__status-title all-same system-status-report__status-icon system-status-report__status-icon--error"> </span>
    +        <span class='error er-wr-detail'> {{ error }} {{'error'|t }}</span>
    +      </span>
    +      <span class="all-exits system-status-report-counter">
    +        <span class="system-status-report__status-title all-same  system-status-report__status-icon system-status-report__status-icon--warning"></span>
    +        <span class='warning er-wr-detail' >{{ warning }} {{ 'warning'|t }}</span>
    +      </span>
    +      <span class="all-exits system-status-report-counter">
    +        <span class="system-status-report__status-title all-same system-status-report__status-icon system-status-report__status-icon--checked"></span>
    +        <span class="checked er-wr-detail">{{ checked }} {{ 'checked'|t }} </span>
    +      </span>
    +    {% elseif error != 0 or checked != 0  %}
    +      <span class=" half-width system-status-report__status-title system-status-report__status-icon system-status-report__status-icon--error"> {{ error }} {{'error'|t }} </span>
    +      <span class=" half-width system-status-report__status-title system-status-report__status-icon system-status-report__status-icon--checked">{{ checked }} {{ 'checked'|t }} </span>
    +    {% elseif warning != 0 or checked != 0 %}
    +      <span class=" half-width system-status-report__status-title system-status-report__status-icon system-status-report__status-icon--warning">{{ warning }} {{ 'warning'|t }}</span>
    +      <span class="system-status-report__status-title system-status-report__status-icon system-status-report__status-icon--checked">{{ checked }} {{ 'checked'|t }} </span>
    +    {% else %}
    +      <span class="full-width test system-status-report__status-title system-status-report__status-icon system-status-report__status-icon--checked">{{ checked }} {{ 'checked'|t }} </span>
    +    {% endif %}
       <tbody>
    

    Should spans be between the table and tbody? That looks odd to me. These should be above the table, no?

    Also, watch your indentation here too, compare to indents used in the file.

rdellis87’s picture

Attended my first Drupalcorn Drupal Camp and my first Sprint. Looked into this with hedrickbt at the Sprint.

Sumit kumar’s picture

@Gábor Hojtsy,@hedrickbt Thanks for reviewing my patch. I am working on it and will resubmit patch with correct indentation.
@hedrickbt,@Gábor Hojtsy Please suggest me if there any other way to do this code better.

{% if error != 0 or warning != 0 or checked != 0 %}
...
+    {% elseif error != 0 or checked != 0  %}
...
+    {% elseif warning != 0 or checked != 0 %}
...
+    {% else %}

waiting for your inputs.

Thanks in advance.

Gábor Hojtsy’s picture

+++ b/core/themes/stable/templates/admin/status-report.html.twig
@@ -16,6 +16,29 @@
+   {% if error != 0 or warning != 0 or checked != 0 %}
...
+    {% elseif error != 0 or checked != 0  %}
+      <span class=" half-width system-status-report__status-title system-status-report__status-icon system-status-report__status-icon--error"> {{ error }} {{'error'|t }} </span>
+      <span class=" half-width system-status-report__status-title system-status-report__status-icon system-status-report__status-icon--checked">{{ checked }} {{ 'checked'|t }} </span>
+    {% elseif warning != 0 or checked != 0 %}
+      <span class=" half-width system-status-report__status-title system-status-report__status-icon system-status-report__status-icon--warning">{{ warning }} {{ 'warning'|t }}</span>
+      <span class="system-status-report__status-title system-status-report__status-icon system-status-report__status-icon--checked">{{ checked }} {{ 'checked'|t }} </span>
+    {% else %}
+      <span class="full-width test system-status-report__status-title system-status-report__status-icon system-status-report__status-icon--checked">{{ checked }} {{ 'checked'|t }} </span>
+    {% endif %}

Yeah, @hedrickbt is right. The last two conditions would match even if only one of the two is non-zero, eg. only the items checked is non-zero and the errors and warnings are 0. I think you wanted and conditions. For the first case if all three are non-zero, for the second two cases if there is an error or there is a warning on top of checked items and the last one is the else case.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Sumit kumar’s picture

Status: Needs work » Needs review
FileSize
8.8 KB
Gábor Hojtsy’s picture

@Sumit kumar: it would be important to post interdiffs, so we can see what changed in your work. See https://www.drupal.org/documentation/git/interdiff

Attached are some direct fixes.

1. Simplified the PHP code, since we are already doing a loop on the requirements, we can just use that.
2. Renamed 'checked' to 'info' in terms of internal variable naming since that is what it is in the existing API, we still report it as 'checked' on the UI.
3. Added docs on new variables to the twig file.
4. Fixed the twig logic. As multiple people pointed out, all the 'or' should have been 'and'.
5. Fixed the CSS indentations that are inadvertently added. I pointed this out in #184 and it was not fixed.
6. Added the /* LTR */ marker to things that were missing them.
7. Added one missing RTL style.
8. Also added the twig updates to the base module style.

I am not sure twig file changes like this are allowed, it certainly does not conform to stable being stable if we are making changes like this. So we may need to rename the stylesheet and start using a new one.

The highlight items are still not implemented but should be. Also CSS needs serious review, classes like "all-exits" don't seem right to me.

Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
13.32 KB

The summary looks good even in responsive settings. The responsive behavior has some issues when the toolbar menu is open on the side though:

lauriii’s picture

Assigned: Unassigned » lauriii
Status: Needs review » Needs work

I'm working on a small clean up for the frontend implementation

lauriii’s picture

Assigned: lauriii » Unassigned
Status: Needs work » Needs review
FileSize
10.75 KB
15.08 KB

What I did on my patch:

  • Moved CSS / markup changes from stable and system to Seven theme
  • Simplified class and CSS structure to match with the Seven standards
  • Started using flexbox
Sumit kumar’s picture

I have review patch #193 and its working fine. @lauriii i have concern only with one that is "counter". if we can replace it with "system-status-report_counter" then its fine i think.

lauriii’s picture

Issue tags: -Amsterdam2014, -DevDaysMilan

@Sumit kumar Thank you for the review! I liked at first the idea of using simply the counter as a name for the component but I do understand your concern about using that name because it is quite generic. How would you like if we simply just rename counter to "system-status-counter"?

dawehner’s picture

Component: base system » system.module

Sorry for the noise. I totally love the initiative to make that, quite often used, page nicer and easier to understand.

Sumit kumar’s picture

yes @laurii we can proceed with it.

Sumit kumar’s picture

Added @laurii patch and cloned the status-report.html.twig and name in status-report-summay.html.twig in both place(one is core\modules\system\templates and another is in core\themes\stable\templates\admin).

joelpittet’s picture

FileSize
4.92 KB

@sumit kumar, I agree with @lauriii and you, the name "counter" may be a bit too generic for the component and his name "system-status-counter" makes sense. Here's an interdiff (the changes between the patch in #193 and one #198).

Please put those up so we can all follow the changes easier. If you haven't made one before as Gabor mentioned in #190 refer to https://www.drupal.org/documentation/git/interdiff

Here's an interdiff for between #193 and #198 on your behalf.

Regarding the testbot failure in #198, it looks just to be a library change caused that, you've added the css/components/system-status-report-counters.css and css/components/counter.css to the libraries file but the actual files missed it into the patch. Could you add them please?

Just spotted this issue, this looks amazing, nice work all!

joelpittet’s picture

Sorry had a .gitignore issue(was accidentally excluding paths with themes/ in them) for that interdiff, bad example to show you, this one would be more accurate.

joelpittet’s picture

@Sumar kumar, not sure about the template name change, if we can avoid that it would be better. I reverted that change to get the styles working again, can you explain the need for that a bit more as I didn't see anything mentioned about that until your change?

Here's the class name change hope that is getting on the right track? The styles look the same as they did in #193.
Hopefully will pass tests.

I'm going to try to get more of the styles into status-report.html.twig

joelpittet’s picture

Status: Needs work » Needs review
joelpittet’s picture

Using #172 as reference I started splitting variables up.

I addressed

Moved CSS / markup changes from stable and system to Seven theme

further from @lauriii in #193 by moving all the markup changes from system/stable to Seven theme.

  1. Removed the table and replaced with grouped details elements(for collapsible)
  2. Split out variables from the requirements that will be in the "General System Information", using the title in the seven preprocess (title was the best identifier I could think of, and compared the untranslated version)
  3. Removed the background colors as mentioned in #172 are not needed (and their hover)
  4. Removed the CSS from system.admin.css by overriding stable's version in Seven and moving all but the "Status report" section that was removed in previous patches.

Feel free to take this further, @Gábor Hojtsy's comment from #191 still need some addressing and there is much more stuff to style now from the design.

@ckrina, the design change between mobile togglable details and desktop not, may be a bit tricky and the arrows on the right are different from the rest of the details which have arrows on the left, any chance you can make a tweak there to make the implementation a bit easier?

Hope that helps move this along.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
21.85 KB
7.39 KB

Not sure what to do with the 'ok' status, just added it to the group sorting to appease the testbot.

  1. Re-worked the counter styles for hopefully better mobile and closer match to the design.
  2. Made the media query mobile first (min-width).
  3. Added the details links and tried to make sense of the details class, so put it on the anchor link.

The trouble with #191 is that the toolbar adds 240px when it's not fixed and vertical, which is kind of a PITA.

joelpittet’s picture

Here is the button for "Run cron" added and slightly styled. Also tried to add that box-shadow and icon background pale yellowish color like the comps.

Add some screenshots of the state it's in so far:

Mobile

mobile

Desktop

desktop

joelpittet’s picture

Playing with flexbox a bit:) A bit closer...

desktop

Ok I'm tapping out.

Still missing:

  1. grey SVG icons in general info
  2. The error and warning icons on the detail summaries
  3. The word "found" after warning and errors
  4. Cron button positioning for desktop
  5. Disable details toggling on desktop
  6. Testing and the things I didn't notice
joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +CSS

Feel free to continue working on and discussing the missing items from #208

yoroy’s picture

Thanks for pushing this forward @joelpittet! Who's next? :-)

ckrina’s picture

@joel sorry I missed the message. I think what you did in your last commit is the best solution to make it easier to implement: moving the arrow to the left and removing the icon. There is still the text color to pick out the kind of message, and they are also separated. So it's still easy to see the difference.
And removing the behaviour on desktop I don't think is necessary. I just added it to make the behaviour simpler, but clearly it doesn't worth so much work because it does little.
I totally agree with the change, so I will update the designs to keep the summary updated.

joelpittet’s picture

Thanks ckrina, that will help implementation a bunch I'm sure!

dscoop’s picture

This update will be appreciated so much by a lot of people! Really cool!

ckrina’s picture

Updated summary designs.

marcvangend’s picture

Really nice to see the progress here, thanks all!

Minor nitpick: in status-page__mb--toggle-left.png (the mobile status page) the string "2048M" was accidentally placed under WEB SERVER instead of PHP>Memory.

chrisrockwell’s picture

Taking a stab at this. Hopefully I didn't mess up the interdiff/patch, first time trying an interdiff.

I started with the icons in the general info section.

A couple things I noticed (I did attempt to follow all the comments, hope I'm not being redundant here):

  • On narrow screens the collapsible elements text is being hidden
  • "Last Cron Run Version" should be "Last Cron Run"?
  • Plural versions for status counters? e.g. if there are more than 1 errors it reads "5 Error". I'm very new to twig, not sure if there is some sort of plural filter but I'll look into it.

I'm also attaching screenshots of what the differences are.

chrisrockwell’s picture

Status: Needs work » Needs review

I guess I set this to Needs Review so tests can run, then set back to Needs Work?

chrisrockwell’s picture

Not sure how the .gitignore got in there, sorry about that. Trying again.

chrisrockwell’s picture

This one adds icons, background colors, and bottom borders to the status report entries. I've also added a wrapper around each section of entries (each section contains 1 h3 and 1+ details elements).

I found that the background colors were removed from colors.css. To match the mockups I needed to add them in, but I just did it in system-status-report.css - are they better in colors.css?

Finally (and I'm working off of Joel's list in 208), I can knock out the details being open on desktop but I wanted to make sure I went about it the correct way. As a d8 newb I think what I should do is add another library (drupal.details-open), attach it within the twig template, and using something along the lines of (done properly, within an attach method on a behavior of course):

      var $statusDetails = $(context).find('.system-status-report__entry');
      console.log($statusDetails);
      if ($statusDetails.length) {
        var desktop = window.matchMedia('(min-width:48em)');
        if (desktop.matches) {
          $statusDetails.attr('open', 'open');
        }
      }

I appreciate any feedback/guidance/suggestions. Thanks!

chrisrockwell’s picture

This one is the JS for opening/closing the details based on screen size. I do have a couple questions about the UX:

Right now, on resize, any with "aria-pressed=true" will not close. However, I'm not sure how to handle this on desktop - it currently just opens them all, even if a user had explicitly opened one or more. I could see this causing confusion when A browser window is narrow, THEN user clicks to open one with lots of information (e.g. node access permissions), and drags the window out. One solution might be to not open any when the screen is widened *if* the user has already taken an action to open any of them.

Thanks!

joelpittet’s picture

@chrisrockwell, thanks for picking this up!

Re #222 The background colors were removed on purpose from colors, see screenshots from ckrina in #214.
The attaching another JS behavior totally sounds like the right way to do that, give it a shot:)

Re #216

  1. "On narrow screens the collapsible elements text is being hidden" - whoops, didn't notice that
  2. ' "Last Cron Run Version" should be "Last Cron Run"?' - Thanks that was a copy/paste mistake by me:)
  3. Plural versions for status counters? e.g. if there are more than 1 errors it reads "5 Error". I'm very new to twig, not sure if there is some sort of plural filter but I'll look into it. This can be done with the trans tag example :
    {% trans %}
      Hello star.
    {% plural count %}
      Hello {{ count }} stars.
    {% endtrans %}

Icons are looking great!

chrisrockwell’s picture

Thanks @joelpittet - the %trans% seems to be working great. I went right over the color changes, sorry about that, so I've moved it out.

This is more of a cleanup patch with one exception:

  • Remove background colors
  • Reduce size of icons in General System Information
  • Destroy the evidence of copying nav-tabs.js for reponsive-details.js
  • Make > 1 Errors/Warnings plural in the counters
joelpittet’s picture

Status: Needs review » Needs work
FileSize
357.2 KB
136.26 KB

Destroy the evidence

lol

This is looking great, thanks for the improvements. I made some notes in the following screenshots:
mobile
mobile

I'm kind of hoping we don't need the intro copy.
The biggest thing that needs a bit of help is the arrow to summary spacing.
Would like some direction from @ckrina on the scrolling cron info, and the top right positioned button and that cron link which is not in the design.

ckrina’s picture

What I thought about that long cron link is to not repeat it here since we already have it on the cron page. I was thinking in a link to that cron page (just like the one in PHP) with the text "more information".
There were so many things to decide it went unnoticed that just now I realized we didn't talk about it.

What I proposed is to not repeat here this information because this box should be something quick to read, with a minimum of info/text. And that link is just one click away.
Another option would be to create some kind or collapsible behaviour, but once it's opened it would create the scroll anyway.

And about the "Run cron button", it's on the right because it fills the gap on the left by created by not having the third column in this row. Is that too difficult to archive? Should I search another place for it?

chrisrockwell’s picture

@joelpittet Thanks for your notes. Re: the collapsible elements - I failed to check browsers that need the poly-fill. The attached patch addresses that (not sure if I should just hide #224).

This is what it should look like:

@ckrina I'm new to this issue (and not sure overall of the process of deciding whether to remove information or not), but I love your idea of not displaying the additional info here and adding the "(more information)" link. Positioning the cron button is easy enough to do.

I'm going to keep working through the list, which I'll update here (items with strikethrough may still need to be reviewed, but they're represented in patches):

  1. grey SVG icons in general info
  2. The error and warning icons on the detail summaries
  3. The word "found" after warning and errors
  4. Cron button positioning for desktop
  5. Disable details toggling on desktop
  6. Testing and the things I didn't notice
  7. Missing Intro copy (#227)
  8. Toolbar vert issue (#227)
  9. Cron information (#227 and #228

Thanks!

chrisrockwell’s picture

Status: Needs work » Needs review
FileSize
41.27 KB
2.96 KB

Forgot the patch and interdiff.

chrisrockwell’s picture

This one addresses "The word 'found' after warning and errors" and "Cron positioning for desktop".

I used seven_preprocess_status_report() to change up the cron text object. Now I see that the link was added to system.install within this issue. This would be much cleaner if we can just change it in system.install to make the link available as it's own variable (system.run_cron is what I'm using now). I'm only realizing this (that it was added in this issue) now but when I first analyzed this task I thought, "if I could just change it in system.install this would be much faster; BUT, what other themes are depending on it being there?" Now I realize the answer is very likely, "zero". Hindsight and all that.

@joelpittet I see the Intro text on my install, it's within the help region though, and not being displayed within status-report.twig.

chrisrockwell’s picture

I'm not going to make the test pass before hearing back on whether or not we can just change system.install

chrisrockwell’s picture

Status: Needs work » Needs review
FileSize
41.89 KB
4.54 KB

Can't resist. It's so much better without the preprocess stuff.

oriol_e9g’s picture

 .system-status-general-info__item-details {
...
   width: 100%;
   max-width: 80%;

widht 100% and max-width 80%? :p

chrisrockwell’s picture

Thanks @oriol_e9g, I've removed it :).

The attached patch adds the (more information) link to the Cron section by creating another variable in the cron render. Right now, I'm hiding the description via .visually-hidden - I think this is a good way to go.

This patch is failing locally for me, I'm assuming it will fail here as well, but I'm hoping someone can take a look at the system.install interdiff and tell me why the "Run cron" link, which is tested on line 124 of Drupal\system\Tests\System\CronRunTest->testManualCron() is failing.

chrisrockwell’s picture

Issue summary: View changes

Adding to-do list to Remaining tasks

oriol_e9g’s picture

I think that the text "Memory" should be "Memory limit", it's a text more accurated and according with our help pages like: https://www.drupal.org/docs/7/managing-site-performance-and-scalability/...

lauriii’s picture

Oh wow! This is starting to look amazing! Good work everyone!

  1. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,225 @@
    +
    +
    

    Nitpick: Unnecessary double line change

  2. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,225 @@
    +.system-status-general-info__item-details h3 {
    

    Could we add class for the h3 element so we don't need to do unnecessary nesting?

  3. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,225 @@
    +a.details-title {
    

    Is there a reason why we specify that details-title has to be an a element?

  4. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,225 @@
    +.system-status-report__status-title .details-title:before,
    

    I couldn't find the .details-title class anywhere, could someone help we figuring out where can I find it?

  5. +++ b/core/themes/seven/css/components/system.admin.css
    @@ -0,0 +1,356 @@
    + * Modules page.
    

    Why are we adding styles for modules page here?

  6. +++ b/core/themes/seven/js/responsive-details.js
    @@ -0,0 +1,47 @@
    +   * Initialise the details JS.
    

    We could add some documentation according to the Javascript API documentation standards: https://www.drupal.org/node/2183405

  7. +++ b/core/themes/seven/js/responsive-details.js
    @@ -0,0 +1,47 @@
    +  Drupal.behaviors.responsiveDetails = {
    

    We should ask one of the JS maintainers to take a quick look on this


  8. For some reason, there is two cron buttons visible for me

  9. On RTL the cron buttons don't float nicely. Also the Icons on the counters are aligned a bit awkwardly when using RTL.

  10. Also the system status tables are not floating correctly on the RTL
Gábor Hojtsy’s picture

+++ b/core/themes/seven/seven.theme
@@ -145,6 +145,110 @@ function seven_preprocess_maintenance_page(&$variables) {
+    switch ($title) {
+      case 'Drupal':

I made some important remarks about this two months ago in comment #159. I don't think switching by title is a good way to go since the titles may be altered. It would be more practical to introduce a new highlight key in the requirements array with these names 'version', 'cron', etc. and pick them out based on that instead of the more fragile title matching. If we decide to simplify the title of some in the future, it may break the status page otherwise for example.

joelpittet’s picture

@chrisrockwell re #234

Can't resist. It's so much better without the preprocess stuff

I agree, and thanks for adding the tasks to the summary.

@oriol_e9g re #241 I agree, I'll add limit in the next patch.

@lauriii re #242 thanks for the review. Responses:

  1. Thanks
  2. Yes
  3. Probably not, the class can likely stand on it's own
  4. .details-title is created by the collapse.js
  5. It's overriding a core CSS file to remove some styles, @see core/themes/seven/seven.info.yml
  6. Thanks for pointing that out
  7. I agree
  8. Thanks for checking RTL
  9. Thanks for checking RTL

@Gábor Hojtsy re #243 I agree, a key would be better, I'll see what I can do.

chrisrockwell’s picture

I was stopping for the night but I see @joelpittet's comment - before you put in any effort please see if you can use anything from the attached interdiff (between 234 and now) - I was working from @lauriii's comments. and have the rtl stuff worked out (I think) along with a start on the JS comments.

Here are the comments I was tracking during work, they should map to @lauriii's feedback.

  1. Removde extra linebreaks
  2. Removed specificity of a on details element
  3. It's added by collapse.js, which is a polyfill for the element
  4. Looks like a chmod change - not sure what that has to do with this, if anything
  5. Got started on this, feedback is appreciated
  6. Tag with needs JS maintainers feedback?
  7. Sorry, debug code when I was trying to make the tests pass locally
  8. Ready for review
  9. Ready for review

I'll be back tomorrow!

joelpittet’s picture

Status: Needs work » Needs review
FileSize
15.25 KB
44.01 KB

@ckrina re #228 That seems logical to not repeat it, it was kind of handy but the logic you put forth seems reasonable so let's go with it until someone says otherwise.

@chrisrockwell re #231 thanks for the detail about the help, I had help module turned off.

  1. I refactored the JS, hopefully it's up to standards, yes could use JS review.
  2. I changed the requirements sorting by weights to preserve keys because we had keys and they just got trashed by usort(), does that work @Gábor Hojtsy? It does clean the preprocess up quite a bit!
  3. Hopefully covered all the RTL issues
  4. Removed the duplicate Run cron button
  5. I added some string translation to the text we added to the Twig template.
  6. I changed the responsive details to use calc() instead of percentage to bring the summary labels closer to the text, hope that's ok @chrisrockwell? or should we revert that?
  7. Reverted the "Found" bit because it may not be needed, and it needs the plural treatment anyways. @ckrina is that ok or is the 'Found' text important?
joelpittet’s picture

@chrisrockwell sorry cross-posted with you! I'll try to merge stuff if possible

joelpittet’s picture

@chrisrockwell, sorry again, here's an interdiff with all the things I could merge. Hope I got them all, your responsive changes covered them much better than mine.

Didn't mean to step on stuff you were doing, just trying to get that error out of the way (which is because we removed button from the description which breaks other themes), and that item @Gábor Hojtsy mentioned about titles as switch. I was just doing the CSS in passing. Feel free to continue kicking ass on this @chrisrockwell!

yoroy’s picture

Just a note to say I'm really happy to see this friendly and constructive back and forth. At almost 250 comments in that is no small feat :-)

Thanks for pushing this forward all!

borisson_’s picture

Status: Needs review » Needs work

I have nitpicks, sorry.

  1. +++ b/core/modules/system/system.admin.inc
    @@ -146,7 +146,7 @@ function template_preprocess_status_report(&$variables) {
    -  foreach ($variables['requirements'] as $i => $requirement) {
    +  foreach ($variables['requirements'] as $key => $requirement) {
    

    This seems an unrelated change, let's revert that part.

  2. +++ b/core/themes/stable/images/core/icons/cccccc/clock.svg
    @@ -0,0 +1,7 @@
    +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
    +	 width="42.659px" height="46.603px" viewBox="0 0 42.659 46.603" enable-background="new 0 0 42.659 46.603" xml:space="preserve">
    
    +++ b/core/themes/stable/images/core/icons/cccccc/d8-logo.svg
    @@ -0,0 +1,23 @@
    +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
    +	 width="47.411px" height="53.531px" viewBox="0 0 47.411 53.531" enable-background="new 0 0 47.411 53.531" xml:space="preserve">
    
    +++ b/core/themes/stable/images/core/icons/cccccc/database.svg
    @@ -0,0 +1,26 @@
    +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
    +	 width="38.847px" height="44.262px" viewBox="0 0 38.847 44.262" enable-background="new 0 0 38.847 44.262" xml:space="preserve">
    
    +++ b/core/themes/stable/images/core/icons/cccccc/php-logo.svg
    @@ -0,0 +1,20 @@
    +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
    +	 width="66px" height="33.447px" viewBox="0 0 66 33.447" enable-background="new 0 0 66 33.447" xml:space="preserve">
    
    +++ b/core/themes/stable/images/core/icons/cccccc/server.svg
    @@ -0,0 +1,16 @@
    +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
    +	 width="44px" height="35px" viewBox="0 0 44 35" enable-background="new 0 0 44 35" xml:space="preserve">
    

    The svg's here seem to have tabs in them, should those be replaced with spaces? Not sure if that's needed as they are probably generated.

ckrina’s picture

@joelpittet I don't think the "found" word is a critical thing. I just added it to give some context and make it easier to understand what kind of info shows. But I think it's easy to understand anyway, so removing it seems reasonable for me.

chrisrockwell’s picture

Issue summary: View changes
chrisrockwell’s picture

sorry again, here's an interdiff with all the things I could merge. Hope I got them all, your responsive changes covered them much better than mine.

Didn't mean to step on stuff you were doing, just trying to get that error out of the way

No problem at all, just didn't want you to reproduce anything that you could copy/paste. Thanks for explaining the error, I wasn't thinking about other themes.

I'll take a look at the vertical toolbar issue. I took a stab at it the other day and I think the easiest fixes would be to either a) decrease the font size or b) kick the media query in a bit sooner. Unless someone can get element queries through quickly :).

chrisrockwell’s picture

The svg's here seem to have tabs in them, should those be replaced with spaces? Not sure if that's needed as they are probably generated.

@borisson_ I think that's me, I copied them into that folder.

joelpittet’s picture

@ckrina, thanks, the word found could be done, just needs to be done with plural forms, and my brain isn't remembering the best way to do that. @chrisrockwell, if you know, feel free to add it back in.

@borisson_ regarding 1), I did that because I didn't want people to think it was an integer anymore and that it was an associative key as originally defined, it's not totally necessary and we can revert if you think that's over-reaching?

@chrisrockwell, I won't step on the patch tonight, feel free to tackle borrison's feedback. And the vertical toolbar issue I wouldn't want to hold this up because of that, so we can make that a follow-up and get it close enough there. Maybe kick the media query in a bit sooner and call it a day/till next time? Element queries FTW, let's fix the browsers too!

chrisrockwell’s picture

This fixes up the svg's and the issue with the counters when the vertical toolbar is open. I'll be the first to admit I'm not thrilled about the solution because it uses the toolbar classes. I didn't want to blanket lower the font size just for the times the toolbar is open (I am pretty diligent about closing it, not sure about the majority of users).

I haven't reverted #2 in @borisson_'s feedback only because, in my newb opinion, that work made it easier to follow along with what is going on, and $key is more representative of what is happening. I'm happy to create another patch reverting it though - just let me know.

I think we still need:

  1. @Gábor Hojtsy to review in regards to #159
  2. A JS maintainer to review responsive-details.js
  3. Add the word "found" back in, with plural form?

Joel - we cross-posted again, I'll take a stab at the "found" tomorrow if it's decided to be better.

chrisrockwell’s picture

Status: Needs work » Needs review
chrisrockwell’s picture

Issue summary: View changes
nod_’s picture

Issue tags: +JavaScript
nod_’s picture

Status: Needs review » Needs work

Missing a couple of things in the JS:

  • it is potentially a problem to open/close details with the open attribute instead of a click on the summary. With the click the aria attributes are properly set. If someone could test the acessibility of this solution that'd be great.
  • we need to have a dedicated data attribute to select elements from JS we can't rely on class names. Insead of .system-status-report__entry, we could use details after all it's just on this page that we mess with this. If an attribute is needed use data-drupal-selector like: <details data-drupal-selector="status-report-details">
  • Need a .once, $(window).once('responsive-details')
  • core/jquery.once in JS dependencies.

Sorry to put that NW but the selector thing is an issue see #1090592: [meta] Use HTML5 data-drupal-* attributes instead of #ID selectors in Drupal.settings for context.

lauriii’s picture

I put a little bit more effort to review the RTL this time. Good work everyone, especially @chrisrockwell and @joelpittet are rocking this issue right now ;)

  1. +++ b/core/themes/seven/css/components/system-status-counter.css
    @@ -0,0 +1,87 @@
    +  border-right: 1px solid #e6e4df; /* LTR */
    

    I was wondering in general, how closely do we want to follow SMACSS standards in Drupal core? Do we want to separate the colors etc under the theme category?

  2. +++ b/core/themes/seven/css/components/system-status-counter.css
    @@ -0,0 +1,87 @@
    +  background-color: #FAF9F5;
    
    +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,240 @@
    +  background-color: #F5F5F2;
    
    +++ b/core/themes/seven/css/components/system.admin.css
    @@ -0,0 +1,356 @@
    +  color: #5C5C5B;
    

    According to Drupal CSS coding standards, colors should be always written in lowercase https://www.drupal.org/node/1887862#properties

  3. +++ b/core/themes/seven/css/components/system-status-counter.css
    @@ -0,0 +1,87 @@
    +  font-size: 16px;
    

    I'm not pointing out all the places where this is wrong but I think we need to convert all the px units to rems and use the px as a fallback. More information: https://www.drupal.org/node/1887862#properties

  4. +++ b/core/themes/seven/css/components/system-status-counter.css
    @@ -0,0 +1,87 @@
    \ No newline at end of file
    

    Newline missing from the end of file

  5. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,240 @@
    +  padding-left: 10px;
    

    This is missing RTL

  6. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,240 @@
    +    right: 1em;
    

    Missing LTR comment

  7. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,240 @@
    +.system-status-report__status-title {
    ...
    +[dir="rtl"].details  .system-status-report__status-title {
    

    Why are we using different selector for the RTL styles here? It would be also nice if the RTL styles would be placed right after the LTR styles

  8. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,240 @@
    +  padding: 1em 1em 1em 3em;
    

    This is missing LTR comment

  9. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,240 @@
    +  padding-left: 3em;
    

    Missing LTR comment

  10. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,240 @@
    +.collapse-processed > .system-status-report__status-title:before {
    +  float: right;
    +}
    +.system-status-report__status-title::-webkit-details-marker {
    +  float: right;
    +}
    

    These could be optimized to one selector. We should also add the LTR comment.

  11. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,240 @@
    +[dir="rtl"] .collapse-processed > .system-status-report__status-title:before {
    +  float: left;
    +}
    +[dir="rtl"] .system-status-report__status-title::-webkit-details-marker {
    +  float: left;
    +}
    

    These could be also optimized to one statement

  12. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,240 @@
    +.system-status-report summary:first-child ~ * {
    +  display: none;
    +}
    +.system-status-report details[open] > *,
    +.system-status-report details > summary:first-child {
    +  display: block;
    +}
    

    This looks like something that would break very easily when something is being refactored. If there is no other way to do this, could we at least add comment what is the expected behavior.

  13. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,240 @@
    +  left: 10px;
    

    Missing LTR comment

  14. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,240 @@
    +  margin-right: 10px;
    

    Should we also add RTL styles for the margin?

  15. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,240 @@
    +  padding: 0 1em 1em 3em;
    

    Missing LTR comment

  16. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,240 @@
    +    width: 35ch;
    

    Why are we using ch here? According to our CSS formatting guidelines we should be using rem

  17. +++ b/core/themes/seven/css/components/system.admin.css
    @@ -0,0 +1,356 @@
    +    float: left;  /* LTR */
    

    Nitpick: Double space before the comment


  18. There is still some issues with the titles on the rtl mode. I think the title should be on right.

  19. The text on the list items is not aligned properly vertically
chrisrockwell’s picture

Status: Needs work » Needs review
FileSize
44.9 KB
1.82 KB

The JS stuff is new to me so I'm trying to wrap my head around it. I looked at several implementations of once() and it seems like using it on $statusDetails would be that right way. @nod_ - was it your intention that it was used how you put it in your comment?

Regarding aria-* I took a look at the specs on those as well. What this patch does is set aria-expanded=true but does not use a click. The reason I chose this way is because using a click also sets aria-pressed=true which isn't accurate, as far as I understand the intention.

I am passing debounce into the function now, as I found that was a pattern being used throughout other core js files.

I'll work on the CSS feedback from @lauriii as well, but I wanted to separate these two out due to my inexperience with js in d8. Looking forward to feedback!

chrisrockwell’s picture

@lauriii I have tried to address all of the feedback but it's possible I missed something. I have a couple comments/questions:

3. Does this apply to dimensions as well?
7. Because different styles are applied for rtl based on whether the polyfill is used:

[dir="rtl"] .system-status-report__status-title .details-title {
  padding-right: 3em;
  padding-left: 0;
}
[dir="rtl"].details  .system-status-report__status-title {
  padding: 1em 3em 1em 1em;
}

10, 11. When I originally did this I thought the same thing, but I couldn't get it to work. When I tried to use comma separated selectors the poly-filled version wouldn't work.
12. I'm going to come back to this one. I believe this is a standard way of making sure elements work cross browser but I'll need to look more.
16. I had to lookup what ch was when I saw it in there :) It solved the problem so I'll let someone with more knowledge than me comment :)
19. I can't reproduce this, what browser is this happening in?

I really appreciate your feedback - it's helping me dig into the docs and standards more - but I will need your eyes on this again to make sure I covered everything that hasn't been noted above. Thanks!

chrisrockwell’s picture

Re: 12 in #261..In my testing this is necessary for poly-filled details and elements to behave correctly so I added a comment and some spacing around it.

Re: 18 in #261 I have changed it to rem. I'm wondering, do we even need px fallbacks for rem units anymore? http://caniuse.com/#feat=rem

Re: #19 @lauriii are you using IE 11 there? Apparently it calculates differently than other browsers: http://caniuse.com/#search=ch

oriol_e9g’s picture

Ultraminor:

Duplicated width:

+  width: 2.815rem;
+  width: 45px;
+  width: 2.815rem;

Three diferent units in a operation is dificult for me to understand the result.

+    width: calc(100% - 35ch - 1em);
lauriii’s picture

#263.3 I'm pretty sure it applies to font-sizes only

#264.18 This would be a good discussion. Probably we should bring that up somewhere to get rid of that requirement. For now I still think we need it.

#264.19 I'm using Chrome but I couldn't reproduce the problem with the latest patch anymore. Something must have fixed it in the meanwhile.

We should still address #263.16. I tested the latest patch and both, LTR and RTL modes are working! Thanks @chrisrockwell for sticking around! It's amazing to have this moving forward. Good work!

I'm removing some tags that are not relevant anymore.

We still need accessibility review on this as @nod_ mentioned in #260.

I'd like to do another code review for this one still. I don't think it's necessarily hard blocking this issue since we are still waiting for the accessibility review. I will try to find some time to do it this week.

mgifford’s picture

We're just linking to 'Details' 3 times. Let's make sure this is pronounced correctly to screen readers by using this instead:

<a href="#error" class="system-status-counter__details"><span class="visually-hidden">Error </span>Details</a>
<a href="#warning" class="system-status-counter__details"><span class="visually-hidden">Warning </span>Details</a>
<a href="#info" class="system-status-counter__details"><span class="visually-hidden">Checked </span>Details</a>

I'd also like to see this text:

<div class="description">There was a problem checking <a href="/admin/reports/updates">available updates</a> for Drupal. See the <a href="/admin/reports/updates/update">available updates</a> page for more information and to install your missing updates.</div>

Go to something like this:

<div class="description">There was a problem checking <a href="/admin/reports/updates">available updates</a> for Drupal. See the <a href="/admin/reports/updates/update">available updates page</a> for more information and to install your missing updates.</div>

So again we aren't using the same text to link to two different places. I'm not sure that this is the best way to summarize this, and it's probably something that is out of scope, but thought I'd raise it.

Mostly I'd like to say that this looks like a great improvement!

chrisrockwell’s picture

Thanks all!

Next up we have:

  • Get rid of duplicated width #265
  • Address use of ch #265, #263.16 (I'm going to suggest we just take it out but, again, I wasn't aware of the unit until this
  • Both points in #267 sound good.

I'm going to wait on another review from @lauriii but if you don't find time later in the week I'll update the patch to address the above.

Sumit kumar’s picture

Hi @chrisrockwell Thanks for contribution i had reviewed your patch. Need to add vendor prefixes for box-sizing.
box-sizing: border-box

Thanks

joelpittet’s picture

Assigned: Unassigned » lauriii

My thoughts on ch was that it's a good unit if your aiming for rough character count. The mixed calc was (100% - rough size of the box - margin/padding). I'm not married to it, was showing off:P

@Sumit kumar, I don't think we need vendor prefixes, which one needs support that we are missing? http://caniuse.com/#feat=css3-boxsizing

Assigning to Lauri for review

Manjit.Singh’s picture

Issue tags: +Dublin2016
chrisrockwell’s picture

This patch addresses #265 by removing duplicate width (found in other places as well) and simplifying the calc(). I also removed the ch unit. Finally, I implemented #267.1 but didn't do #2 yet. @mgifford mentioned it might be out of scope and it's in update.module, which hasn't been modified in this issue, so I'll leave it to someone more knowledgable than me to give the go-ahead.

I don't think prefixes are needed on box-sizing either, I'm checking caniuse.com and http://shouldiprefix.com/ (which might use caniuse.com)

Hope everyone that is in Dublin, is enjoying it! And everyone that's not, is enjoying their time also :)

Sumit kumar’s picture

@chrisrockwell i found one issue. Please see the attachment

Sumit kumar’s picture

FileSize
16.62 KB
136.94 KB

Fixed the issue i have added this code. In system-status-report.css file.

@media screen and (min-width: 48em) and (max-width: 60em) {
  .toolbar-tray-open.toolbar-vertical .system-status-general-info__run-cron {
    position: relative;
    margin-bottom: 10px;
  }
}
chrisrockwell’s picture

Thanks @Sumit kumar. I'm attaching a re-rolled patch with your changes along with an interdiff. Maybe your work wasn't applied against the latest patch.

Sumit kumar’s picture

Thanks @chrisrockwell yes my changes are there.

Sumit kumar’s picture

Reviewed the patch working fine.

vulcanr’s picture

Status: Needs review » Needs work

The patch above is working fine, but if I'm asked, I really think that for mobile we can really push a little bit more, some details (since we are already working on this and is not too much work to get there):

- Hide the Title of the Blocks (General System Information)
- Reduce the fonts a little bit, I think that the font is a little bit overwhelming on the titles.

That's it.

ckrina’s picture

Status: Needs work » Needs review
FileSize
13.17 KB
16.62 KB
37.15 KB

I'm attaching a patch with new SVG version already compressed and some minor fixes. Also I think is good to say here that I made the icons for the server.svg, the database.svg and the clock.svg and you can use/modify/whatever for whatever you need (https://opensource.org/licenses/MIT). The other ones are the D8 logo and the PHP logo (license here).

I also found a minor issue on the CSS regarding to Run cron button:
Run cron button

The padding comes from the class .no-touchevents .button--small, and maybe we could just avoid the padding completely.

Thank you @vulcanr for your comments. I agree with you that hiding the Title of the Block (General System Information) is a good improvement. We would have less information in small displays.
And I also agree about changing the font sizes, but I think it should be addressed on another issue because it should happen around the site. If we change the font sizes only here we wouldn't be maintaining the consistency across the site, so I vote for moving forward with this issue and addressing it in another one. But it's a really good point to keep in mind.

And it's exiting how the issue is moving forward! Thank you all for the work :)

joelpittet’s picture

Assigned: lauriii » joelpittet
Status: Needs review » Needs work

Going to tackle the JS/CSS toggling and button style.

joelpittet’s picture

Assigned: joelpittet » Unassigned
Status: Needs work » Needs review
FileSize
43.29 KB
1.97 KB

This addresses the button left/right margin by removing it, adds an event handler to prevent the click toggle for details, and hid the "General System Information" title.

vulcanr’s picture

@ckrina absolutely in regards to the font size (need another issue for that, hope can be tackled quite fast - I could take care of that here in Dublin).

Going to test now the patch, will let you know in a few minutes about it.

vulcanr’s picture

Status: Needs work » Needs review

The patch 286 works fine for me. I will change the Status to "Need Review" someone else can test it again and change the status.

FYI @ckrina @joelpittet

Manuel Garcia’s picture

Status: Needs review » Needs work

Patch is looking really nice, good work @all!
JS is good to go imho.
I've not reviewed all the css although it looked fine at a first glance.

Small nitpick on the PHP side:

+++ b/core/themes/seven/seven.theme
@@ -145,6 +145,97 @@ function seven_preprocess_maintenance_page(&$variables) {
+    if (isset($requirement['severity'])) {
+      $severity = $severities[(int) $requirement['severity']];
+    }
+    elseif (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'install') {
+      $severity = $severities[REQUIREMENT_OK];
+    }
+    else {
+      $severity = $severities[REQUIREMENT_INFO];
+    }

Can we avoid the else statement by initializing $severity to be $severities[REQUIREMENT_INFO]; by default - Just makes it easier to follow

ckrina’s picture

FileSize
118.72 KB

I think now we have a problem with the Details & Summary elements because they aren't working properly on the last Firefox release (49) and IE/Edge: http://caniuse.com/#search=details. The arrow to point the details can be opened just disappeared.

Arrow disappeared.

So, we could just move forward anyway and hope this browsers start supporting this in a short time or we could hide the default webkit arrow and add another one with CSS. Even it only appears on small sizes and IE/Edge/Firefox and they are not that popular in smaller devices, I would go for this last one. I think it is a necessary visual pointer to show there is more information, otherwise some users may lose it.
Just to point it, I used the twistie-down.svg icon from Stable theme in the design and converted it to black.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
43.27 KB
747 bytes

Addressing #291

vulcanr’s picture

@ckrina can we add instead of svg icons, I think we could play around with ::after elements in both browsers. Just an idea.

vulcanr’s picture

Just tested both Chrome&Firefox, and I can see the arrows in both of them mobile.

And as #291 (Manuel) said, the patch works fine, look&feel fine.

I think that for a next issue (really low priority, and could be a Novice issue for Tomorrow - Mentored Sprints) we could remove the underlined text when the box is open.

lauriii’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
41.08 KB
2.94 KB
97.46 KB

Great work with this issue. I'm sorry to say but I think there's still some things that might need some work. Anyway I'm happy to help with this!

  1. The margin between the counters and the genereal system information is too small
  2. We need followup to figure out what to do with the button component

  3. We will need to figure out how do we manage the counters when the admin toolbar is on the side. Currently, we change the counter component styling depending on whether the toolbar is open or not. I would suggest that all elements are as self-contained as possible, and they shouldn't be affected by other elements. For that reason, it would be good to create some other way to manage the open toolbar scenario.
  4. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,277 @@
    +    display: flex;
    

    It would be good to see some pictures taken on IE 11 since flex is only partially supported there. See: http://caniuse.com/#feat=flexbox

  5. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,277 @@
    +[dir="rtl"].details  .system-status-report__status-title {
    +  padding: 1em 3em 1em 1em;
    +}
    

    The selector is wrong for this RTL selector. It doesn't need to be more specific than the selector for LTR styles. The LTR styles should be also followed directly by the RTL styles.

  6. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,277 @@
    +.system-status-report summary:first-child ~ * {
    ...
    +.system-status-report details[open] > *,
    +.system-status-report details > summary:first-child {
    

    Is there a reason for this specific selectors? I feel like we could use way less specific selectors on this.

  7. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,277 @@
    +.system-status-report__status-title .details-title:before,
    +.details .system-status-report__status-icon:before {
    

    Could we always set this styling for the summary element instead of setting it for different element depending on whether there is polyfill or not?

  8. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,277 @@
    +  .system-status-report details > summary {
    ...
    +  .system-status-report details > summary:hover {
    ...
    +  .system-status-report details > summary:first-child {
    ...
    +  [dir="rtl"] .system-status-report details > summary:first-child {
    

    Is there a reason for using this specific selectors here?

  9. +++ b/core/themes/seven/js/responsive-details.js
    @@ -0,0 +1,44 @@
    +          // If user explicitly opened one, leave it alone.
    

    This comment should answer for the question "why?"

  10. +++ b/core/themes/seven/seven.theme
    @@ -145,6 +145,96 @@ function seven_preprocess_maintenance_page(&$variables) {
    +function seven_preprocess_status_report(&$variables) {
    

    I'm really worried about how complicated the status_report related things are since we are actually ouputting multiple things inside one template. The template is very complicated.

    We should create new templates for counters, general system information and the table.

    This all could be put together inside a render element or something.

    I'm very sorry to bring this up this late, but I'm not comfortable with this at all. This is not a good example for anyone looking at how to do these things themself. I'm also happy to help fixing this.

vulcanr’s picture

Front End table working on this at DrupalCon Dublin 2016

vulcanr’s picture

FileSize
236.33 KB
300.82 KB
204.19 KB
264.92 KB

Here are a couple of screenshots in different resolutions (1920 and 1440) with the toolbar open and closed. I set the font size when the toolbar is open and when it's closed. I find it a little bit "glitchy" to see the size of the font change when opening the toolbar.
Note: Adding the patch in the next comment.

vulcanr’s picture

vulcanr’s picture

Assigned: Unassigned » vulcanr

Good thing the test didnt pass :) (didnt finish cleaning up the code)...

Anyways the patch contains font-size: 16px all the time, toolbar shown or not, it's 16px. I agree with lauriii @ #296, shouldn't be different.

I will assing this task to myself, just waiting input from ckrina in regards to design.

ckrina’s picture

I'm trying this last patch but it looks like something is missing. I guess so also because @Manuel Garcia's patch was 43.27 KB and the last one is only 16.99 KB. @vulcanr is that possible that you didn't add all the files in this last one?

vulcanr’s picture

I will try to reproduce it, and will let you know @ckrina

vulcanr’s picture

Added the new patch for this.

Had to recreate again the fixes i implemented before (maybe i forgot to add the files, to be honest can't recall it).

Manuel Garcia’s picture

FileSize
1.66 KB

Thank you @vulcanr!

Here is an interdiff between previous patch and #305.

prabhu9484’s picture

@vulcanr - please attach the interdiff for the above patch

vulcanr’s picture

@prabhu9484, Manuel already added the interdiff in the comment above.

Gábor Hojtsy’s picture

+++ b/core/themes/seven/templates/status-report.html.twig
--- /dev/null
+++ b/core/themes/stable/images/core/icons/cccccc/clock.svg

+++ b/core/themes/stable/images/core/icons/cccccc/clock.svg
+++ b/core/themes/stable/images/core/icons/cccccc/clock.svg
@@ -0,0 +1,3 @@

--- /dev/null
+++ b/core/themes/stable/images/core/icons/cccccc/d8-logo.svg

+++ b/core/themes/stable/images/core/icons/cccccc/d8-logo.svg
+++ b/core/themes/stable/images/core/icons/cccccc/d8-logo.svg
@@ -0,0 +1,6 @@

All the styling/template changes are in seven, why are the SVGs in stable? AFAIS they should be in Seven, no?

lauriii’s picture

#309 +1, that way we can also keep them as internal and re-iterate easily on them at any point

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
212.48 KB
116.89 KB

Tested the patch found these three problem:

1. The header text is not as designed:

2. I see there is a hover underscore behavior on the info when on a small screen. This somehow happens on a wide screen too under some circumstances, eg. when hovering over the header (I did not find this consistently reprodusible after resizing the screen multiple times though):

3. The anchors work well for error/warning/info, BUT unfortunately the header and the first info is hidden behind the toolbar when the anchor jumps there because the anchored point is obviously the top of the screen. Not sure what to do about this or if we can do something simple about this, but its confusing for sure.

ckrina’s picture

About #311 and the anchors point 3: we found that issue during the DrupalCon with @joelpittet. We created another issue #2808699: Offset the html anchor to adjust for Toolbar to try to solve it independently, but it looks like it's not that simple. Anyway, I wouldn't try to solve it here because it is caused by the Toolbar.

Sumit kumar’s picture

@Gábor Hojtsy point number 1 and 2 is done submit patch and interdiff file

vulcanr’s picture

Hi all, sorry didn't have time to answer.

I will take care of this issue during the weekend. Hope to have some early feedbacks next week.

@ckrina +1 i think the toolbar is another issue, from this issue, we can (already did) guarantee the look of the report with the toolbar open/closed (when it's on the side on small resolutions).

@#311 to be honest, didn't take a look to the rest of the report, just realized what people commented about the top part, mainly on the 3 blocks where the issues are and the mobile polishing. But is not going to be a big deal to fix that (thanks for the feedback).

I will move the folder for the icons (#cccccc) to the seven folder (no problem).

Cheers guys!

Gábor Hojtsy’s picture

@Sumit: I don't think you changed all three labels. Also the ones you did change do not use the text from the design. Apparently the existing labels are also tested, so the tests need to be adjusted too.

vulcanr’s picture

Ok guys, new patch added, in this patch I think are covered the missing styles for this report page:

- Relocated the icons folder inside the Seven Theme.
- Added background color for the status blocks and for the general system information.
- Added background colors for the status on "Errors found" and "Warnings found"
- Edited the strings for both cases "Errors found" and "Warnings found"
- Increased the padding between the Errors, Warnings and Checked blocks.
- I don't know if I'm the only one that saw this, but the General System Information has some border radius at the top of it.
- Changed the font-size for the General System Information header.

If something else is missing. Let me know!

Br.

vulcanr’s picture

Gabor, in regards to #316, I think that a different font was used in the design and there is a discussion at #1986082: Add a webfont version of Source Sans Pro font that mentions a font change in Seven. I don't think that it's smart mix those 2 things. In the entire design the font is different than in the 'patch' for example. Just adjusted font-sizes to make it look similar.

Maybe @ckrina or @lauriii could give us some light in here.

Sumit kumar’s picture

@vulcanr thanks Request to attach interdiff file.

lauriii’s picture

@vulcanr: I think there are some unrelated changes in the patch on #317. Would you by any chance be able to remove them?

I agree with #319. Let's not touch the font-family for now.

ckrina’s picture

FileSize
68.88 KB

I agree with #319 & #321 on keeping font decisions outside this issue. Just as information, I used Lucida Grande on the design because it's what is being used now.

What I'm realising now is that there is more information in the general info block:

PHP info in the block

This block is intended to be a "quick info" block, so adding too many text there makes it difficult to get quickly the most relevant information (PHP version). It also makes it more difficult to copy/paste this info to any issue to give quick info about the current installation (#130).
I would suggest to move the text "PHP versions higher than 5.6.5 or 5.5.21 provide built-in SQL injection protection for mysql databases. It is recommended to update." to the bottom list with all the other text/messages.

Gábor Hojtsy’s picture

Huh, where did I write in #316 that a different FONT should be used? All I pointed out was that #313 did not change the label "Info" and the "Warning found" and "Error found" text was neither as designed. Where did I talk about fonts?

Gábor Hojtsy’s picture

Status: Needs review » Needs work
  1. +++ b/core/themes/seven/seven.theme
    @@ -145,6 +145,96 @@ function seven_preprocess_maintenance_page(&$variables) {
    +    REQUIREMENT_INFO => [
    +      'title' => t('Info'),
    +      'status' => 'info',
    +    ],
    

    The design still says "Checked" for these ones, not "Info".

  2. +++ b/core/themes/seven/seven.theme
    @@ -145,6 +145,96 @@ function seven_preprocess_maintenance_page(&$variables) {
    +    REQUIREMENT_OK => [
    +      'title' => t('OK'),
    +      'status' => 'ok',
    +    ],
    

    Its not clear to me what happens with these ones in the patch?

vulcanr’s picture

Sorry I missunderstood your words, I saw your comment in #311 and supposed you were targeting on the String of text that didnt match the design (I thought mentioning design you wanted to reffer to the font as well - my bad).

lauriii’s picture

Assigned: vulcanr » lauriii

I'm working on this

lauriii’s picture

Status: Needs work » Needs review
FileSize
60.15 KB
61.23 KB

I did a major refactoring on the way the markup is created. I also fixed some reported issues and reverted the fonts back.

In this patch I have addressed at least:

We will still need to figure out, what to do with class soup in system-status-report.css.

lauriii’s picture

Assigned: lauriii » Unassigned
Status: Needs review » Needs work

Feel free to grab this, I'm not hammering on this actively now

lauriii’s picture

Status: Needs work » Needs review
FileSize
59.62 KB
393 bytes

This doesn't belong to this patch

The last submitted patch, 327: redesign_the_status-665790-327.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 329: redesign_the_status-665790-329.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
60.4 KB
2.25 KB

Status: Needs review » Needs work

The last submitted patch, 332: redesign_the_status-665790-332.patch, failed testing.

Pages