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

droplet’s picture

I'd suggest move width:33% value in CSS to helper classes. There're 1 column, 2 cols, and 3 cols max. It's more simple & extendable for custom modules.

It's same to buttons inside the grids. Make a standard class and positions for it.

also .system-status-general-info needs overflow:hidden for sub-pixels problem in last col border in particular windows size.

In my point of view, Last Cron Run is more important than Drupal Version in Status Page. I'd place it in first column.

I don't know if anyone interests to place a link or Performance Info / Search Index sections into Status Page. It's more handy tho.

So that,

General System Information - tending to show serverside status (drupal version, web server, php ..etc)
Another Actionable/Site status Section - show the actionable site's status (search index, caching, cron...etc.. It will be extenable to show advanced info, for example using Memcached with action button to clear cache, Using CloudFlare with clear cache button, show site analytics)

lauriii’s picture

@droplet thanks for the comment.

The design has been pretty much decided so I'd rather not change it at this point anymore. If you feel like making some further changes for the design / functionality, feel free to open up a follow-up issue once this issue is finished. :)

I agree that it makes sense to create some generic classes for the most popular use cases.

droplet’s picture

Also, let's prepare a response section for Ajax actions on Buttons (if we intend to do it soon)

** Making it ajaxable is out of the scope of this issue but I think design the UI should be done here.

EDITED: I commented before reading #335

droplet’s picture

@lauriii,

I think the design team could make a quick evaluation. If my new idea is cool and workable. It's good to change it now. We needed a pattern & decision on new UI changes than keeping open new follow-up to revise a new UI in next release. I don't think Durpal is a MVP.

Gábor Hojtsy’s picture

@droplet: thanks for your feedback, the sooner we get this in people's hands, the sooner we can get wider feedback which can help refine the design later based on wider feedback vs. those few people who read the issue. This issue is nearing its 7th birthday :)

droplet’s picture

Yeah. It's comment #339. But nothing can block Good UI & Code, right? It just meaningless to do the same UI with 7 years old efforts twice within a month or so.

If our Core Developers on this issue able to do a quick evaluation, we will save another 7 years on next UI update. We settle it down as an MUST-DO follow-up than a random idea from a developer. (And more important is.. the developers on this issue willing to handle the patch in follow-up than the developer giving the idea).

In this case, I think the Status Page's UI still controllable and predictable. We design future-proof solid patterns :) Then, the Drupal Core Pattern will be another Bootstrap & Zurb Foundation, the Google Material design.

EDITED:

The UI/UX/AI teams knows what they wanted to put on Status Page and don't. I hope it won't spend them a lot of time...

ckrina’s picture

@droplet: thank you for your feedbacks and ideas. I've already heard/read some suggestions/improvements about the current design that could make it much better. But I also think that we should try to finish a minimum viable product and then iterate on it with all the feedback we can get.

@lauriii some icons are missing on the last patches. They were on the #317 patch, and gone on the #327.
Missing icons

lauriii’s picture

Status: Needs work » Needs review
FileSize
64.99 KB
16.41 KB

This should address #340.

Status: Needs review » Needs work

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

joelpittet’s picture

@lauriii, sorry I've not reviewed and responded sooner. The ideas you had were worth discussing but IMO implementing #296.10 derailed the issue a bit, which is on me for not being clear when we discussed it in person at DrupalCon. Since that is quite a huge change in direction it needs more discussion and I'd like to discuss more in IRC/Slack or in person next week? In the meantime could you revert that change and keep your other changes?

Reasons:

  1. There are benefits to 'all in one template': as a themer I don't have to search for another template and hope I don't break something elsewhere if I tweak it the markup. The abstraction into 3 templates when the variables can belong to one template seems unnecessary without a solid re-use-case.
  2. The introduction of the #type and moving it into Stable is a pretty cool trick but needs extra testing and not needed if we work with what we have.
  3. status-report-general-info.html.twig hardcodes the variables into the template where as with one template this could be changed to cover @droplets hierarchy/order concerns very easily and even grab different variables from the list to promote them
  4. The status-report-counter.html.twig anchors are linked in a completely different file, which seems super fragile if we don't know what template those #ids are refering to and visaversa.

I've notes from all the comments from my last patch in #286 to the latest code but would rather post when I can confirm and test the changes if/when reverted template split.

Hopefully those are reasons enough, don't mean to be a downer:(

lauriii’s picture

Thanks for the review @joelpittet!

  • I agree that there is benefits in keeping things in one template, but in this case there's quite a lot of overhead caused by it. The components are a little bit too complex to be duplicated in a way they were. It was difficult to see the differences and things in common between the different elements.

    Also, this template is already being re-used by core on the updater, so we have to be careful with the changes we make. The approach of having all in one template is also very much against the more component based approach we are trying to take in Drupal core. Even though this is not component-based theming, I'd like to try not to make it the opposite of that.

    Also, these elements are very self-contained on the front-end so they shouldn't break each other very easily.

  • This is not a new issue caused by my changes. Logic needs testing. Now that it's separated from the markup we can actually even write Unit tests on that.
  • These changes could be still managed by modifying the template and making changes for the render element. This can be done just by adding additional variables without removing any of the existing variables.
  • That is very valid concerns and therefore should be documented on the template.

Anyway, not depending on which approach we take, we have to be able to cover other use cases this is used for. That's why I'd like to suggest that we create a new status-report-page template which would load all these 3 elements. It could be done either using includes or theme implementations. Then we could keep the status report template itself reusable.

lauriii’s picture

Status: Needs work » Needs review
FileSize
67.55 KB
19.59 KB

Discussed this issue with @joelpittet and @Cottser and we agreed that we want to have separation between status report page and the requirements table because the requirements table is being re-used in core. I created another render element which is called status_report_page which does the status report page specific lifting.

Status: Needs review » Needs work

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

lauriii’s picture

Status: Needs work » Needs review
FileSize
68 KB
1.47 KB

The same table and Seven styles are applied for update.php requirements table also. I changed the update.php styles slightly but this might need some more work.

Status: Needs review » Needs work

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

lauriii’s picture

Status: Needs work » Needs review
FileSize
68.87 KB
5.89 KB

We will also need to figure out what to do with the installer. It also uses the status report table from Seven.

Status: Needs review » Needs work

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

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

lauriii’s picture

Status: Needs work » Needs review
FileSize
68.91 KB
482 bytes
chrisrockwell’s picture

From the perspective of a user I've tested this pretty thoroughly and it seems to be excellent - I do like the separation. The only issue I could find is that the details element marker is no longer present in Firefox 49 (currently 49.0.2) because they added support for details so no more polyfill (!). More info on that in #2821525: Update normalize.css to 5.0.0. Not sure if we want to add that normalize.css fix in now or wait to see if #2821525 makes it in.

All that being said, we'll need to style the Firefox details marker to match the design, we have ::-moz-list-bullet to do that with.

If anyone has insight on how to position the ::moz-list-bullet please chime in - I'm unable to get it to move :). In order to achieve consistency cross-browser we may need to use a background image for the details list-item marker.

droplet’s picture

  1. +++ b/core/themes/seven/js/responsive-details.js
    @@ -0,0 +1,44 @@
    +            .on('click.details-open', false);
    ...
    +            .off('.details-open')
    ...
    +      $(window).on('resize.details', debounce(handleResize, 150)).trigger('resize.details');
    

    Needs namespaced with seven theme than global.

  2. +++ b/core/themes/seven/js/responsive-details.js
    @@ -0,0 +1,44 @@
    +      $(window).on('resize.details', debounce(handleResize, 150)).trigger('resize.details');
    

    Considered listen on Media Query than resize event.

  3. +++ b/core/themes/seven/seven.libraries.yml
    @@ -75,6 +78,16 @@ drupal.nav-tabs:
    +  dependencies:
    

    missing collapse.js

Manuel Garcia’s picture

+++ b/core/themes/stable/stable.theme
@@ -23,3 +32,70 @@ function stable_preprocess_links(&$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 using this else statement by initiating the $severity variable before the if / elseif? (nitpick, but easier to read)

$severity = $severities[REQUIREMENT_INFO];

chrisrockwell’s picture

  1. +++ b/core/lib/Drupal/Core/Render/Element/StatusReport.php
    @@ -0,0 +1,91 @@
    +    $severities = static::getServerities();
    

    Misspelling? Maybe should be getSeverities()

  2. +++ b/core/modules/system/src/Element/StatusReportPage.php
    @@ -0,0 +1,139 @@
    +    $severities = StatusReport::getServerities();
    

    Same misspelling

  3. +++ b/core/themes/seven/css/components/system.admin.css
    @@ -0,0 +1,356 @@
    +@media screen and (min-width: 45em) {
    +  body:not(.toolbar-vertical) .system-themes-list-installed .screenshot,
    +  body:not(.toolbar-vertical) .system-themes-list-installed .no-screenshot {
    

    Any interest in styling for the toolbar instead of negation?

  4. +++ b/core/themes/stable/stable.theme
    @@ -23,3 +32,70 @@ function stable_preprocess_links(&$variables) {
    +  $severities = [
    +    REQUIREMENT_INFO => [
    +      'title' => t('Info'),
    +      'status' => 'info',
    +    ],
    

    Can we use StatusReport::getServerities() instead?

I can drop the suggested changes into a patch with interdiff if no one is actively working on this.

chrisrockwell’s picture

I'm working on this..the attached patch addresses

  • #356.{1,2,4) - fix misspellings, use static method StatusReport::getSeverities()
  • #354.2 Listen for MQ (Are there any benefits to using resize with debounce here?. Media Query matching appears to be faster: https://jsperf.com/matchmedia-vs-resize/6 (~5% faster than jquery resize) and we already have a polyfill for addListener).
  • #354.3 I believe collapse is required for nav-tabs and responsive-details

Comments:

  • Does nav-tabs need to be namespaced?
  • Re: #355 IMHO when an elseif() {} is necessary _and_ a fallback will be set, it's easier to read if final else {}sets the default.
chrisrockwell’s picture

+++ b/core/themes/seven/js/nav-tabs.js
@@ -44,6 +44,7 @@
+        console.log($tabs);

Remove debug code

droplet’s picture

#354.2 Listen for MQ (Are there any benefits to using resize with debounce here?. Media Query matching appears to be faster: https://jsperf.com/matchmedia-vs-resize/6 (~5% faster than jquery resize) and we already have a polyfill for addListener).

debounce is a performance trick to reduce the callbacks on resize. Just a suggestion and I don't have a strong opinion on it. We will never hit bottleneck in current usage

Does nav-tabs need to be namespaced?

the event name needs namespaced, e.g.: resize.detail

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

    this is a new script. I'd say this also needed namespaced.

  2. +++ b/core/themes/seven/js/responsive-details.js
    @@ -0,0 +1,44 @@
    +      function handleResize() {
    

    this func should move out of `attach` also

Manuel Garcia’s picture

Re: #357 on #355 - fair enough

Sumit kumar’s picture

Status: Needs review » Needs work
FileSize
47.82 KB

I have tested patch no #357 So i found one issue regarding to border right for more detail see the attachment. I have test this issue on 1366 with normal mode(100%).

Thanks

chrisrockwell’s picture

chrisrockwell’s picture

No reason to hide the most recent patches, I don't think.

pguillard’s picture

Status: Needs work » Needs review
FileSize
68.57 KB

Applied suggestions at #368 and #359-2 (Don't see what #359-2 means)

Concerning #361, yes, border-right is missing in some short intervals, but globally it is there for mobile/desktop/tablet normal resolutions.

chrisrockwell’s picture

Status: Needs review » Needs work
FileSize
2.09 KB

Attaching an interdiff between #357 and #364. There was a regression in JS as the details no longer close correctly at the breakpoint. Have to run now but I'll take a look when I get back.

Thanks!

chrisrockwell’s picture

  1. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,141 @@
    +  .system-status-report details > summary {
    +    cursor: default;
    +  }
    

    I'm not sure it's a good idea to try to disable or otherwise change the default behavior even if on wide screens.

  2. +++ b/core/themes/seven/js/responsive-details.js
    @@ -0,0 +1,46 @@
    +          .on('click.details-open', false);
    ...
    +          .off('.details-open')
    

    I'm not sure why we're messing with the events here. I *think* it's intended to disable the behavior on wide screens but I don't see any benefit in that. It ends up causing buggy behavior if a user:
    - On narrow screen opens a details element
    - makes screen wide
    - makes screen narrow
    - tries to close it - it won't close

    Granted, probably a .0001% edge case, but still buggy.

chrisrockwell’s picture

This patch addresses the JS regression and is refactored to be more consistent with nav-tabs.js. I also added changes for my comments in #366.

chrisrockwell’s picture

Status: Needs work » Needs review
Sumit kumar’s picture

Hi i have tested patch #367 and its working fine.

ckrina’s picture

Status: Needs review » Needs work
FileSize
186.62 KB

Sorry, I've found a little issue in larger screens: there's an overlap of status titles/elements when we click to open them. I attach a gif to show it.

Overlap on titles

chrisrockwell’s picture

Thanks @ckrina.

This looks like an easy fix but I'll also note that @joelpittet brought up in slack that work here was causing issues in the installer with the seven theme and there was, I think, talk about stripping out some of the details customizations here. I think I can find time today/tomorrow to test it out there and try to come up with a plan - I'll report back.

Gábor Hojtsy’s picture

Where do we have the list of outstanding tasks? The issue summary has all crossed over. It would be nice to get this in sooner than later. Its nice that we are making it super-perfect, but at the same time IMHO we need to ask the question if the patch is making the status page worse or better and if its better, will it be carved in stone or still improved later on? If it can be improved later too, then why hold up a positive change? :)

Note that I do agree to fix known bugs with the new implementation but perfecting it to death does not help much :)

Does the "Needs markup review" tag still stand, does this need markup review?

Does the "Needs accessibility review" tag still stand, does this need accessibility review still?

lauriii’s picture

I removed needs markup & accessibility review tags since we are not ready yet for these reviews to happen.

I updated some of the points I know we still need to fix before this is ready to the issue summary.

@joelpittet: I remember you had a great list of outstanding issues so maybe you could check if you some extra points that still needs to be solved.

chrisrockwell’s picture

Status: Needs work » Needs review
FileSize
4.14 KB
141.14 KB

This patch only addresses #349 because I don't know if this is the right way to do it, so I want to isolate it.

The jist of it: use preprocess to set a variable if we're currently in the install_verify_requirements state and then set some classes in status-report.html.twig.

Setting to Needs Review so the tests can run, but need to switch it back to Needs Work even if they pass.

chrisrockwell’s picture

@pierremarcel brought it to my attention in Twig slack channel that 8.3.x had the `create_attribute` function added (Change record: [#2818293]). That is a better solution than using removeAttribute().addAttribute().

chrisrockwell’s picture

Due to needing to make adjustments on the update page also I'm torn between how to do this. AFAIK we'd need two preprocess hooks (1 for install, 1 for update) to add relevant modifier classes for this.

Would it be better to change system.module, line 207 to this:

'status_report' => array(
      'variables' => array(
        'grouped_requirements' => NULL,
        'modifier_class' => NULL
      ),
    ),

And then the other places that need the status report can use:

$build['status_report'] = [
  '#type' => 'status_report',
  '#requirements' => $requirements,
  '#modifier_class' => 'install' // (or maintenance, or update, or whatever)

Another option is to key down on the maintenance-page and then use just one preprocessor - I haven't figured out if that's feasible.

chrisrockwell’s picture

I found the simplest solution I could for fixing the display of <details> elements on install and update. I believe this patch addresses #349, #370, and #371 (at least to the extent of the information I have on them).

This does not address:

  • Remove all usages of toolbar-vertical class
  • CSS clean-up (BEM conversion / remove class nesting)

Status: Needs review » Needs work

The last submitted patch, 377: redesign_status_report-665790-377.patch, failed testing.

ckrina’s picture

I've tested it and the open/close behaviour for details has appeared again in the Status Page itself on desktop devices. It starts on the patch from #367.

chrisrockwell’s picture

Thanks @ckrina. I was resolving the buggy behavior in your gif on #370. I specifically added back in the details behavior, see my comment in #366. If it should be there I can add it back, but I don't see a reason to disable default functionality in this case.

ckrina’s picture

Sorry @chrisrockwell, I didn't noticed your comment in #366. I added the difference in the behaviour during the first design because I supposed it is useless on big screens. I updated the designs on #211 because I don't think it's a really important thing, but someone came up with a solution at some point. But if it's adding some buggy behaviour I'm with you on removing it so we can have an MPV soon.

Gábor Hojtsy’s picture

Ok there seem to be two tasks left in the issue summary:

  1. Remove all usages of toolbar-vertical class
  2. CSS clean-up (BEM conversion / remove class nesting)

Who can help with these? It would be a really big shame to throw out the work that went into this by neglecting this issue and letting it die because it was not 100% perfect.

kostyashupenko’s picture

Assigned: Unassigned » kostyashupenko
chrisrockwell’s picture

Thanks again @ckrina. The buggy behavior will probably only be reproduced by curious developers and designers :) but I think it's ok to just leave the default behavior and if someone clicks on it they can.

Re #382 @Gábor Hojtsy. I was looking for feedback (in Slack) regarding:

  • How far reaching should removal of .toolbar-vertical be? i.e. just seven/css/components/system.admin.css or all system.admin.css files (core/modules/ and stable)
  • CSS clean-up...Are we only focusing on seven here, or system and other base themes as well?

Thanks for taking this on @kostyashupenko!

Chris

kostyashupenko’s picture

Assigned: kostyashupenko » Unassigned
Status: Needs work » Needs review
FileSize
139.08 KB
3.55 KB

i removed toolbar-vertical class usages and made some clean up of css.
Btw, about toolbar-vertical classes, there were some conditions, related to situation only when toolbar is oriented vertically. So maybe need to recreated these styles in other place somewhere.

Also, i have a some thoughts about css

1. First of all - it is calc function. I'm sure there will be tonns of issues, related to ie9 sometimes and some Android 4 maybe. Please pay your attention on it http://caniuse.com/#feat=calc

.system-status-general-info__item-details {
  box-sizing: border-box;
  display: inline-block;
  width: calc(100% - 60px);
  padding-left: 10px; /* LTR */
  position: relative;
}

2. Flexboxes. I didn't test it, but again. Pretty bad browser support of it. Need double check current styles on andro-phones, ie9, Edge, etc. http://caniuse.com/#search=flexbox

...
.system-status-general-info__items {
   display: flex;
   flex-wrap: wrap;
}
.system-status-general-info__item {
   flex: 1;
   flex-basis: 33%;
   width: 33%;
}
.system-status-general-info__item:nth-child(2) {
   flex: 2;
   flex-basis: 66%;
}
...

Need more review of i. Also need double test current styles

chrisrockwell’s picture

Thanks @kostyashupenko. Regarding .toolbar-vertical we can't just remove the classes, see the attached screenshot. I had done work on this but was awaiting feedback on which files should be affected by our changes; I've since deleted that environment (don't ask :D) Can you test this out on the appearance page and let me know if you see differently? Thanks!

droplet’s picture

Issue tags: +Needs JS testing

How far reaching should removal of .toolbar-vertical be? i.e. just seven/css/components/system.admin.css or all system.admin.css files (core/modules/ and stable)

the report page only.
I have a glance at the source code. Nothing needs to be removed.

CSS clean-up...Are we only focusing on seven here, or system and other base themes as well?

All related to report page

......about @kostyashupenko css points

To me, we don't need to these new CSS rules. But I know somebody has diff thoughts here.

(My patch review on next comment)

chrisrockwell’s picture

@droplet, IIRC @lauriii's comments were added after we removed .toolbar-vertical from the status report - that's what led me to believe this is beyond the status report only. Additionally, work has been done on the appearance page in this issue (and the install and update).

Tagging with needs maintainer feedback also so we can get definitive direction on this (I'm hesitant to do any thing else until we know what the goal is).

Gábor Hojtsy’s picture

Additionally, work has been done on the appearance page in this issue (and the install and update).

Why are we doing those changes? The issue summary does not explain the need for them or the nature of them. Even within the scope of this issue, this seems to be contentious enough for whatever technical reasons :/

droplet’s picture

FileSize
63.56 KB
12.86 KB

1.

\core\modules\system\templates\status-report-counter.html.twig
\core\themes\stable\templates\admin\status-report-counter.html.twig

Same files, diff indentation.

2.
\core\themes\classy\templates\admin\status-report-counter.html.twig
This is different than #1 files. I don't know what make these differences but with a new design. I think we better keep it same as possible as we can.

and many of similar structures. I understand we have clean code in system base, but a pretty messed..
classy / seven / system

sometimes files placed at seven only:
\core\themes\seven\templates\status-report.html.twig

3.

3.1
system-status-counter system-status-counter--error
Consider to remove: We don't style ".system-status-counter--error" in core

span or div? div is preferred I think

3.2
the detail link should NOT a part of .system-status-counter__status-title

3.3
.system-status-counter__details -> .system-status-counter__status-details / system-status-counter__details-link

4.

.system-status-report__entry__value

double ___ double __, bad practice

5.

Either .system-status-report__entry--warning OR .color-warning

6.
system-status-report__entry system-status-report__entry--checked color-checked
Same as #5. Also, I don't know what is .system-status-report__entry--checked
If that's default style. To consider remove it.

chrisrockwell’s picture

Why are we doing those changes?

Due to elements such as the status report table, which is used on the install and update page. There was a lot of appearance page css added but I think I jumped onto the issue after that happened.

My main point of confusion, as a developer with very little experience contributing, is where do we stop work on this and then open new issues? I feel like we should stay away from, for example, the toolbar in classy and core, only dealing with Seven.

¯\_(ツ)_/¯

Gábor Hojtsy’s picture

@chrisrockwell: I think there are two ways this issue can go at this point at almost 400 comments. (a) People get scared that the todos are impossible to track, abandon the issue and this never gets committed (b) People decide enough of the perfection and agree the patch can get in finally. I'm trying to keep this spirit alive for (b), so (a) does not happen.

droplet’s picture

@Gábor Hojtsy,

Is it possible to set a new issue as release-blocker? For this case, I think we have no argues on visuals. Markups & CSS & JS are separated for this single report page only. A second round refactoring will not affect other components before D8.3 release.

Less code to review on each change.

Gábor Hojtsy’s picture

@droplet: no, this issue will not get committed if it introduces regressions or things that need to be fixed before a release. Who would sign their oath to make sure that is surely going to be fixed before release? See!

@chrisrockwell: do we have a list of what exactly needs subsystem maintainers review? Who has time to read 400 comments? :)

chrisrockwell’s picture

The way I see it is we are down to 2 to-dos, and both of them need, in my opinion, more clarification (so "needs review" isn't as accurate as "needs feedback"):

  • Remove .toolbar-vertical: there are no references to toolbar-vertical in Seven...where all should it be removed from? Everywhere in core except toolbar proper? (I'm hiding, for now, #385 as it is a regression).
  • CSS clean-up (BEM conversion, remove nesting): Same as above: where? Seven only? Also, how pure do we want this to be before it's acceptable? We could go for only one class per declaration, but is it necessary?

@droplet has some feedback that could be addressed now but, again, I'm personally hesitant to do anything because I'm not sure if it's wasted effort without knowing exactly what the goal here is.

I hope I'm not coming across as a bummer - I'm definitely willing to work more on this but, if it's not clear yet, I'm a bit confused :).

chrisrockwell’s picture

I made a mistake - we haven't done any work on the Appearance page (I don't think :D) - system.admin.css was copied over from system/stable.

I'm under the impression now that the goal is to remove .toolbar-vertical only from seven/css/components/system.admin.css. However, this would amount to us doing work outside of the status report. Cleaning up the CSS in that file could be more difficult and could definitely effect pages outside of the status report.

I have tried to find the nested selectors that were not in system.admin.css to have a record of what to look at as a potential to fix. If the intention is to do the same with system.admin.css there is quite a bit more there. The toolbar-vertical declarations are copied over from system/stable.

@droplet has posted additional feedback in #390

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

    Nested selector

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

    Nested selector

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

    Nested selector

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

    Nested selector

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

    Nested selector

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

    Nested selector

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

    Nested selector

droplet’s picture

Ouch! I think I understand the problem better now!

In this patch, it introduced non-scoped CSS changes. All out of scope code should be removed:

/**
* Appearance page.
*/
	/**
* Modules page.
*/

In another word:

All CSS do not prefix with .system-status need to checked and clean-up!
(Move all non-.system-status prefix to new issue if it's not a regression caused by the PHP code in the last patch)
(** NEW helper .class can be left as it)

chrisrockwell’s picture

I'm starting to wrap my head around this more (thanks to the patience of @Gábor Hojtsy).

Several months ago, around #204 we copied core/modules/system/css/system.admin.css so that we could make changes to .system-status-report. However, we've since moved that into system-status-report.css making, as far as I can tell right now, that move and override unnecessary. I did a quick test and we would need to fix some CSS but I propose:

  1. Delete system.admin.css from core/themes/seven/css/components and remove the libraries override in seven.info.yml
  2. Focus on the css regressions resulting from removing the override - I think is a the width of details elements and font-weight
  3. If we want to remove .toolbar-vertical create a separate issue in core for that

I think this would eliminate a lot of code from this patch (~400 lines) and get focus back on the status report page only. This also clears up my confusion :).

chrisrockwell’s picture

This patch is for my comment in #398.

I hope this can be used as a point for cleaning up the CSS. If so, let's start with #385 and #390.

chrisrockwell’s picture

I'm attaching a screenshot of what Bartik would look like as an admin theme with this patch. This is coming about because of @droplet's comments in #390. We're updating the rendering, providing the proper templates to classy and stable, but only dealing with the presentation in seven.

I don't think this would be considered a bad thing, but I want to note it here.

Gábor Hojtsy’s picture

@chrisrockwell: how much of a regression is this from before the patch? As far as I see the status report page may not be very nicely themed in Bartik before the patch but it is clearly readable. Can we make the version with the patch at least comprehensible even if we don't want to support it as an admin theme or make it support the status page, like we support the node creation page as well?

For reference, this is how it looks like without the patch:

droplet’s picture

Disclosure: I don't understand the methodology of these D8 base theme AI well.

Both Bartik & Seven's base theme is "Classy". Classy is a theme with many classes (With my understanding, this is better styled base theme)

With the Patch:
1. This is API breaking changes. (Including UI)
2. In D8.3, it able to break & change UI of Seven theme
3. In D8.3, it able to break & change UI of Bartik theme also.
4. Every theme using Classy as the base theme should NOT break a lot!

With points above, most of NEW changes in Seven theme should be moved to Classy:

e.g.:
- \core\themes\seven\css\components\....
- \core\themes\seven\js\responsive-details.js

This file can be left if we want to maintain different style to Bartik:
- \core\themes\seven\templates\status-report.html.twig

lauriii’s picture

#402 is wrong. We are not allowed to change Classy at all. Please keep all the changes in the Seven theme and system module. See https://www.drupal.org/core/d8-bc-policy for more info.

droplet’s picture

FileSize
67.98 KB

@lauriii,

Thanks.

If so, it's very messed in D8. I guess that it won't allow patching Classy because we keep it stable.

However, it allowed patching System module. And to patch system module will affect all other contributed themes using Classy. We only fixing Seven & other Core themes. All other contributed themes are broken immediately.

BUT, to provide BC, shouldn't we move new additional files to Classy?


(Bartik with new Seven styles)
Added back these style to every theme using Classy as base theme.

EDITED:

Without patching Classy. Can we call it Classy anymore? Isn't it just a plain Stable theme (for the status page) then?

chrisrockwell’s picture

Re: #401 That is seven before the patch, I'm attaching a screenshot that shows the difference in Bartik - we're only adding information (that is ready to be easily styled). I would consider this comprehensible (and useful information) but if I was a contrib theme maintainer it would be nice to have a heads-up on something like this (not sure how possible that is without them actively following along).

Re in #403 there are templates being added to classy though. However, we'd really be breaking stuff if we did not include those - then the render, I think, would choke in every theme that uses classy as a base when the site upgrades - is that accurate?

+++ b/core/modules/system/templates/status-report.html.twig
@@ -4,38 +4,36 @@
diff --git a/core/themes/classy/templates/admin/status-report-counter.html.twig b/core/themes/classy/templates/admin/status-report-counter.html.twig

diff --git a/core/themes/classy/templates/admin/status-report-counter.html.twig b/core/themes/classy/templates/admin/status-report-counter.html.twig
new file mode 100644

new file mode 100644
index 0000000..5c408b8

index 0000000..5c408b8
--- /dev/null

--- /dev/null
+++ b/core/themes/classy/templates/admin/status-report-counter.html.twig

+++ b/core/themes/classy/templates/admin/status-report-counter.html.twig
+++ b/core/themes/classy/templates/admin/status-report-counter.html.twig
@@ -0,0 +1,27 @@
chrisrockwell’s picture

Marking this as needs review from a subsystem maintainer. Unless I'm missing something, this current iteration simply won't work if we have to limit changes to system and Seven as it will break the status report in stable.

chrisrockwell’s picture

@lauriii commented on Slack that we cannot change Stable either

chrisrockwell’s picture

I think I'm addicted to this issue ;P.

I tracked the issue down to requirements variable not being defined in system_theme(). This patch removes everything related to Classy and Stable and resolves the issue with the status report not showing in themes based on classy or stable.

I'm not sure of an easy way to make an interdiff but I'll work on that next.

chrisrockwell’s picture

+++ b/core/modules/system/system.module
@@ -197,9 +197,32 @@ function system_theme() {
+      'variables' => array(
+        'grouped_requirements' => NULL,
+        'requirements' => NULL,

Adding requirements resolves the issue with status reports on themes that inherit from classy or stable

chrisrockwell’s picture

Already updated the issue queue, removing tag.

@droplet can your work be made less Seven-y?

chrisrockwell’s picture

Somewhere along the road system.install.orig got tracked

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

Status: Needs review » Needs work

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

chrisrockwell’s picture

Per @lauriii (and the failing test) the templates that didn't previously exist in Stable should be included in this patch.

chrisrockwell’s picture

Status: Needs work » Needs review
chrisrockwell’s picture

+++ b/core/themes/stable/templates/admin/status-report-page.html.twig
@@ -0,0 +1,18 @@
diff --git a/core/themes/stable/templates/admin/status-report.html.twig b/core/themes/stable/templates/admin/status-report.html.twig

diff --git a/core/themes/stable/templates/admin/status-report.html.twig b/core/themes/stable/templates/admin/status-report.html.twig
index 7f4c600..95a085a 100644

index 7f4c600..95a085a 100644
--- a/core/themes/stable/templates/admin/status-report.html.twig

--- a/core/themes/stable/templates/admin/status-report.html.twig
+++ b/core/themes/stable/templates/admin/status-report.html.twig

+++ b/core/themes/stable/templates/admin/status-report.html.twig
+++ b/core/themes/stable/templates/admin/status-report.html.twig
@@ -4,15 +4,20 @@

@@ -4,15 +4,20 @@
  * Theme override for the status report.
  *
  * Available variables:
- * - requirements: Contains multiple requirement instances.
- *   Each requirement contains:
- *   - title: The title of the requirement.
- *   - value: (optional) The requirement's status.
- *   - description: (optional) The requirement's description.
- *   - severity_title: The title of the severity.
- *   - severity_status: Indicates the severity status.
+ * - grouped_requirements: Contains grouped requirements.
+ *   Each group contains:
+ *   - title: The title of the group.
+ *   - type: The severity of the group.
+ *   - items: The requirement instances.
+ *     Each requirement item contains:
+ *     - title: The title of the requirement.
+ *     - value: (optional) The requirement's status.
+ *     - description: (optional) The requirement's description.
+ *     - severity_title: The title of the severity.
+ *     - severity_status: Indicates the severity status.
+ * - requirements: Contains list of requirements without grouping.
  *
- * @see template_preprocess_status_report()
+ * @see stable_preprocess_status_report()

These are the changes for the existing template (status-report.html.twig) in Stable - just docblock

droplet’s picture

Status: Needs review » Needs work

Missing some files in the last patch?

Per @lauriii (and the failing test) the templates that didn't previously exist in Stable should be included in this patch.

So basically my comments in #402 is correct. Is it?

chrisrockwell’s picture

Missing some files in the last patch?

Possibly, which ones do you see as missing?

So basically my comments in #402 is correct. Is it?

I don't think so as we aren't changing anything in Classy. The issue now with themes that are based on Classy is they get the counter and general information but no styling (the templates are inherited from system). Stable has to have a copy of any system template files (see StableTemplateOverrideTest::testStableTemplateOverrides) which was a point of confusion for me (I was told a couple times not to change Stable, but I believe the meaning was don't change anything that already exists in Stable.

I think we have the go-ahead from @lauriii to update the docblock in stable->status-report.html.twig

@ckrina is going to try and mock something up that can be used for direction on the default styles. The style declarations should go into system.

Please correct me if I'm wrong, but as long as the patch isn't missing files we are down to two issues:

  1. Default styling of the system status-report* templates for the themes that are based on classy and stable
  2. CSS clean-up and BEM Conversion

It would be awesome if someone more familiar with BEM and D8 css guidelines could look at the classes.

ckrina’s picture

Here are 4 proposals for styling this page on Bartik. I prefer 1&2, specially 1 because it looks like it might be easier to apply and still makes sense.
I tried to use different patterns that I found across different pages like appearance or configuration, but I'm not sure if they are the correct ones for Bartik.

Also @chrisrockwell I tried the last patch and the system-status-counter has lost the margins and the 3 grid positioning on big displays. I attach an screenshot.

chrisrockwell’s picture

Status: Needs work » Needs review
FileSize
56.69 KB

@droplet was right - status-report-page.html.twig was not there. In some cases the template files were only put in classy so when I cleaned out the classy stuff I needed to copy some over to seven, I missed this one.

Should go back to "Needs Work" if tests pass

chrisrockwell’s picture

Forget to mention that #420 should fix #419.

ckrina’s picture

Issue summary: View changes
FileSize
63.57 KB

As discussed in the UX Slack channel, the chosen design for the status page in Bartik should look like the one I'm attaching.

chrisrockwell’s picture

Assigned: Unassigned » chrisrockwell
ckrina’s picture

FileSize
110.36 KB

Sorry, now that we're so close I think I've found what I think is another issue. :)
The contrib modules that add messages into the status page are not showed together with the core messages. The core messages are under "Checked" but contrib messages are under "OK", and all of them should be together under "Cheched". I'm attaching a screenshot with Fitvids and Image Widget Crop as examples.

tkoleary’s picture

Testing #420 in seven on simplytest.me I notice that there are no open/closed arrows on the summary and details elements (checked, warnings etc.) though there is an after pseudo-element there, I'm assuming for that purpose.

Is this a regression in Seven?

There are arrows (on the right) at the mobile media query.

chrisrockwell’s picture

@tkoleary The goal was to not make it look like a details elements as JS is used to open them all when the available width is wide enough. There was JS that disabled the details elements but I removed it as it caused buggy behavior.

tkoleary’s picture

FileSize
1.06 MB

The goal was to not make it look like a details elements as JS is used to open them all when the available width is wide enough.

I see. The behavior is weird though. It really looks like a bug. See video.

It's like the case of the disappearing descriptions. At first I was thinking I turned them off or something, like a checklist. It really needs the arrow affordance or it just doesn't read as expand/collapse.

chrisrockwell’s picture

This patch adds the default styling. The counters in the design were using a Bartik color so I changed them to a system background color.

@ckrina - nice catch! Question is, do we merge REQUIREMENTS_OK into REQUIREMENTS_INFO or maintain a separate section for OK? Personally, I think merging them is fine - the value of this comes when they are REQUIREMENT_WARNING or REQUIREMENT_ERROR so lumping all the checked and ok seems perfectly acceptable to me - what does everyone else think? Also worthy of noting, if we don't merge them into "Checked", we'll need another counter.

@tkoleary I agree, I'll add that back in within another patch.

I found another issue with the default styling though. Within the counters, there are links to "Details" for each group. However, the default render does not group them, it's just a long list (table element). We can change system to group by default but this does nothing for the themes that extend stable as we can't change stable to grouped requirement. Some thoughts:

  • Go ahead and change System to group, issue in Stable to change it.
  • Don't group System, remove the links at the system level and just have them in Seven
  • Link all three to same place - again, we couldn't do anything about Stable here without filing an issue - we'd only be making it useful at the System level

For anyone hopping in late, here is a screenshot of the counters - the "Details" link jumps down the page to the relevant grouping of requirement:
.

And an updated list of to-dos:

  1. CSS cleanup and BEM conversion
  2. Add the details arrows back in on wide screens (#427)
  3. Decide if REQUIREMENT_OK can be grouped with REQUIREMENT_INFO
  4. Decide what to do with "Details" links at system level

Status: Needs review » Needs work

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

chrisrockwell’s picture

Status: Needs work » Needs review
chrisrockwell’s picture

I messed up the library overrides, also including a fix for #427.

chrisrockwell’s picture

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

Un-assigning because I might not have time until early in the week to work on it. If someone wants to move on it, please do so. The CSS clean-up/BEM conversion can be done now before having an answer on #428 3 and 4.

ckrina’s picture

About #428 and the links pointing to the same place, I would vote for the first option (changing System to group too), but maybe it's too complicated and it might have too much implications that I don't know. So maybe the best option is the 2nd one because it looks like it's the easier option.

chrisrockwell’s picture

@ckrina I agree. We can change system but we'd have to file an issue in Stable and Classy to change it there, so any admin themes wouldn't get the grouping until that's done. #2 is a simple solution.

Any thoughts on grouping REQUIREMENT_INFO with REQUIREMENT_OK?

Gábor Hojtsy’s picture

@ckrina I agree. We can change system but we'd have to file an issue in Stable and Classy to change it there

So you are proposing to have regressions once this is committed and a separate issue to resolve that regression. I don't personally think that would be committed. If we can fix it in a separate core issue, why cannot we fix it here honestly?

chrisrockwell’s picture

Quick update: It's been determined that we can and should combine REQUIREMENTS_INFO and REQUIREMENTS_OK.

tkoleary’s picture

+++ b/core/themes/seven/css/components/system-status-report.css
@@ -3,13 +3,138 @@
+    float: right;

Should this have an /* LTR */ ?

chrisrockwell’s picture

Per a discussion in Slack with @Gábor Hojtsy I'm attaching a patch that stops using status-report.html.twig in favor of a grouped status report as the default. This also groups in REQUIREMENT_OK with REQUIREMENT_INFO (#424).

Are we now down to only CSS clean-up/BEM conversion and @tkoleary comment in #437?! :D

chrisrockwell’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

chrisrockwell’s picture

Status: Needs work » Needs review
FileSize
70.56 KB

This is passing locally, re-trying.

Status: Needs review » Needs work

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

chrisrockwell’s picture

Going to try this tests again - without changing anything we have one less fail :)

Status: Needs review » Needs work

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

Sumit kumar’s picture

Status: Needs work » Needs review
vaplas’s picture

Status: Needs review » Needs work

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

giorgio79’s picture

This thread is now officially the longest on d.o. :) https://www.drupal.org/project/issues/drupal?categories=All&text=&status...

Gábor Hojtsy’s picture

The issue summary has:

  • 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)

Is this accurate?

Also what does this need JS testing for? It has the tag but no explanation in remaining tasks.

chrisrockwell’s picture

Issue summary: View changes
Issue tags: +Needs subsystem maintainer review

Updating remaining tasks.

We need to fix the failing test and my implementation of the default display (system and stable) needs to be reviewed.

I didn't add the JS tag, but the JS component that was added is responsive-details.js.

vaplas’s picture

We need to fix the failing test

@chrisrockwell, if you mean the failing between #438 and #443, it has been fixed in patch #446.

Also we have #437 question about LTR.

wturrell’s picture

Non-designer opinion: anyone else think the Details link under system-status-counter looks a bit squashed, compared to the more generous line-height in system-status-general-info? (Blink, Firefox on Mac)

(It looks great btw.)

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

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

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

lauriii’s picture

These are the remaining BEM clean up tasks. If any of these seems difficult, let's clean up them later rather than on this issue. These are in the Seven theme which is internal, which allows us to modify these later.

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

    Can we add specific class for the .details-title?

  2. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,138 @@
    +.system-status-report summary:first-child ~ * {
    

    Can we add class for the summary element?

  3. +++ b/core/themes/seven/css/components/system-status-report.css
    @@ -3,13 +3,138 @@
    +.system-status-report details[open] > *,
    

    Can we add class for the details element?

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

    Can we add class for the .details-title element?

I would also like to propose that we don't add default styles for details on this issue since it could be also done later. We have discussed this on other issues as well, but core modules should not be responsible for providing any design so this wouldn't be a change for that.

I will remove the needs subsystem maintainer tag since system module doesn't have a maintainer. I have also reviewed this patch and it mostly looks very good!

We also still have a Needs JS testing tag on this issue.

lauriii’s picture

Status: Needs work » Needs review

After thinking this for a while, I think we should not block this issue from being committed because of the BEM nitpicks I've made. They are for internal code that can be refactored later if needed. Not fixing these would mainly affect people who are sub-theming Seven, but that is discouraged because Seven is marked as internal. I think it is important to get this committed sooner than later since this already is a way too long issue.

Therefore, this should not be set to needs work anymore but needs review to get the "Needs JS testing" tag away. Other than that, this issue would be ready to be committed IMO. IMHO what we have here is 99%

nod_’s picture

If we have N details elements the script was creating N media query objects and N event listeners. New script create only one. Fixed some selector to make sure we don't end up selecting child details elements by mistake if they are present.

The script affect all details elements on the page, regardless of the page. Do we want to keep that or make sure it only affect details elements on the status page?

In any case the js is good to go now.

leahtard’s picture

I have been working on one other item this Sprint Weekend that I feel will be a UX improvement. I think the removal of the toggle display on wide screens is best as the default expanded state looks a little odd.

I re-implemented the logic from #286 where @joelpitttet initially implemented this. This was removed becasue of the issue mentioned by @chrisrockwell in #336. I have added a fix to responsive-details.js that addresses this use case. I have attached a before/after screenshot for reference (status-before-after.jpg).

Cheers, Leah

Status: Needs review » Needs work

The last submitted patch, 459: core-status-page-665790-459.patch, failed testing.

chrisrockwell’s picture

Status: Needs work » Needs review
FileSize
72.48 KB

I can never reproduce these failures locally; I'm uploading @leahtard's patch as-is to retest

chrisrockwell’s picture

Issue tags: -Needs JS testing

Tests are passing - apparently it is a known intermittent fail with the OutsideInBlockFormTest. Based on @nod_ comment in #458 I think we can remove the Needs JS Testing.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

As per @lauriii and @nod_ this looks good. I reviewed @leahtard's changes and those also looked fine.

Let's get this in finally :)

ckrina’s picture

I've just reviewed @leahtard's changes and everything worked perfect for me too. It's great to have them in the patch!
And regarding the proposed design, everything looks good too.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Reviewed this LIVE on the UX team meeting! (http://youtu.be/X2LJEiW2BS0) Clicked on all of the various jumpy links, tested in RTL mode (works great), and also tested in Stark! Nice work abstracting this out into its own element so it can be reused.

Here were some things that stood out in review:

0) HOLY FRIGGING CRAP THIS LOOKS ABSOLUTELY AMAZING(*&!*@(*!&@(*!&@(*&!@

1) In several places, the templates are using <br> for visual reasons, however this is not semantic. Because we're adjusting core core templates here, it's important to get this right the first time, so we should re-jigger the HTML in these templates so they still create the same visual effect.

2) There are a couple of places where more generic Seven CSS is being adjusted (e.g. color.css and table.css). This seems a bit risky, since it's expected that e.g. sub-themes of Seven would be piggy-backing off of these more generic classes. We should instead override the generic styling in the status page element specifically.

3) In Stark, the warning/error icons show in the details elements below, but not in the big "icons" above. It feels like it should do one or the other in both places.

4) Also in Stark, the details elements are all collapsed by default which makes you slightly insane with trying to get a sense of what's happening. We should probably expand these by default.

5) We're also removing a template preprocess function (-function template_preprocess_status_report(&$variables) {) which is generally frowned upon for BC reasons; however, preprocess functions are explicitly marked as @internal at https://www.drupal.org/core/d8-bc-policy so I think we're ok here.

Overall, these are really small things in the grand scheme of things. We're *really* close on this! Please keep going. :D

leahtard’s picture

I was able to address #1 in @webchicks notes above.

  1. br's have been removed and div's have been added as needed.
  2. Now that we need a wrapper div for .system-status-counter__title-count, I have added 2px of margin to address @wturrell spacing comment in #452
  3. There is still one br being rendered before the "Run cron" button in the "Last Cron Run" block. This is not coming from the templates. Is this coming from the cron.description or cron.run_cron variable?

Cheers, Leah

chrisrockwell’s picture

Leah and I discussed that we were working on the same stuff, I'm uploading my patch here and we can figure out what to merge between the two.

1) <br>'s are removed. I don't pretend to be a front-end developer so I'm not sure my implementation is the best way. Essentially I used <h4>'s when they were sub-headings of h3. For the stable and system themes I simply removed them - I'm attaching a screenshot of what it looks like, I think it looks fine while maintaining the least amount of markup.

2) I added in the removed styles, and overrode them in Seven's css

3) I opted for adding the icons to the counters since the icons existed in the details before this patch. I like the result but it requires adding css to Stable and System and doing overrides in .yml files so we'll see if the testbot approves.

4) This was discussed in Slack and we received approval from @webchick to not do this

5) Per the comment I'm not doing anything with this.

chrisrockwell’s picture

FileSize
74.89 KB

Found a remnant of my testing #4, need to remove "open" from .system-status-report-entry in core/themes/stable/templates/admin/status-report-grouped.html.twig

chrisrockwell’s picture

Status: Needs work » Needs review
chrisrockwell’s picture

Re: #466.3 that is coming from system.install and I missed it also.
+++ b/core/modules/system/system.install
@@ -535,14 +535,19 @@ function system_requirements($phase) {
- '#prefix' => '
',
- '#markup' => t('To run cron from outside the site, go to @cron', [':url' => $cron_url, '@cron' => $cron_url]),
+ '#prefix' => '
',
+ '#type' => 'link',
+ '#title' => t('Run cron'),
+ '#url' => Url::fromRoute('system.run_cron'),

That was previously a
, not sure if we should change this one?

wturrell’s picture

@leahtard #466 - that looks much better, thanks.

wturrell’s picture

It is worth making the icons (.system-status-counter__status-icon) on mobile a bit larger so they are less squashed and vertically align with the images/text underneath (version, cron etc.)? I've only uploaded an interdiff as we have two people working on patches simultaenously…

Before (45px):

Before

After (60px):

After

vaplas’s picture

This patch based on #468 + next ideas:
1. #466: +1 for wrapper system-status-counter__title-count, but it would be better to use <span> intead of <div> for avoid problem with <span><div></div></span> (or replace all parent <span>). Also add <span> wrapper to system template to save a some layout.
2. #472: +1, looks pretty for me!

3. my 2 cent about width and border (see before.png and after.png)

4. +1 for remove <br> before the "Run cron" button, but now it is necessary for narrow screens (see cron_br.gif).

vaplas’s picture

FileSize
75.13 KB

Oops, incorrect patch, sorry!

The last submitted patch, 473: redesign_status_report-665790-473.patch, failed testing.

chrisrockwell’s picture

Removes the <br> from system.install

If we get approval on this change - that gets everything, I think.

vaplas’s picture

Removes the <br> #476 - the perfect solution for me!

vulcanr’s picture

Just tested and seems fine to me.

yoroy’s picture

Status: Needs review » Reviewed & tested by the community

Ok then it's rtbc again!

  • webchick committed 38b3dd1 on 8.4.x
    Issue #665790 by chrisrockwell, Sumit kumar, lauriii, joelpittet, sun,...

  • webchick committed de24348 on 8.3.x
    Issue #665790 by chrisrockwell, Sumit kumar, lauriii, joelpittet, sun,...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Reviewed this again on UX meeting today. WOOHOO! All feedback from earlier is now addressed.

Since this represents a major user-facing change, but only impacts one particular admin screen, it seems worthy of committing to both branches. Thank you so much to EVERYONE who helped out with this amazing issue. This is the #1 most commented on issue in the queue right now, and it's amazing to see it closed. (Famous last words? ;))

Committed 38b3dd1 and pushed to 8.4.x, and cherry-picked to 8.3.x as well. Thanks!

webchick’s picture

Version: 8.4.x-dev » 8.3.x-dev
xjm’s picture

Issue tags: +8.3.0 release notes

Sweet! Totally in favor of backporting this for the alpha; thanks @webchick. Let's also mention it in the release notes/CHANGELOG/etc.!

Does this patch change any strings? If so we should tag as such.

Wim Leers’s picture

EPIC work! :) And such a huge improvement! Thanks everybody!

rootwork’s picture

Congrats everyone! This looks amazing!

Chi’s picture

Status: Fixed » Needs work
FileSize
1.52 MB
855.6 KB
+++ b/core/themes/seven/css/theme/maintenance-page.css
@@ -142,7 +142,7 @@
-    max-width: 770px;

This affected install page as well. See screenshots attached. I suppose it looks even worse on high-resolution screens.

Gábor Hojtsy’s picture

Status: Needs work » Fixed

@Chi: wups, you are right. Can you open another issue, so we have a more suitable way to resolve this issue. I don't think it will need huge changes :) I'll try to make sure it gets attention soon so it will be correct again soon.

yoroy’s picture

Thank you Chi, good catch. Lets create a follow up for that: https://www.drupal.org/node/2854851

Status: Fixed » Closed (fixed)

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

Sumit kumar’s picture

Sumit kumar’s picture

Pages