Problem/Motivation

Drupal's admin UI (the Seven theme) feels outdated and needs a visual update. The current designs were created for Drupal 7 in 2008-2009 and were slightly updated for Drupal 8. At the time, many of the current Drupal's features and components didn't exist and Seven's UX was not taking them into account.

This has been raised by agency leaders as one of the key issues preventing them from selling Drupal. A Drupal admin UI UX study that was done in 2018 also validated the need for a more modern admin UI.

Proposed resolution

Claro has been worked in contrib to allow early adopters to start using pre-release versions of the theme. Claro is also included in the Lightning installation profile which is used by over 3000 sites. As a result, Claro is used by almost 400 sites and has allowed us to gain confidence that the theme, even at its current state is useful. We’ve also received bug reports that have clearly showcased that Claro has been used on production sites.

One of the key reasons why it has been possible to use the pre-release versions of Claro is that Claro has been built on top of Seven. Components or UIs that haven’t received an updated look and feel have remained functional during the whole process.

Claro has been implemented using the new Drupal Design System which has received feedback in the Drupal core ideas queue. To make the designs appeal more modern, the design system is using a color set that is colder as well as provides higher color contrast. The new designs will also make Drupal easier to operate using touch screen devices by increasing the size of all form controls. The new design system will also cover frontend facing components such as Toolbar, Quick Edit, and Contextual Links.

The scope of this first iteration has been limited mainly to consist of visual changes. Later on, we are planning to iterate on the visuals and add improvements to the functionality such as a complete redesign of the file and image upload widgets.

We’ve prioritized pages that are used frequently by content editors: content entity forms and content list and all components on these pages have been redesigned. We’re working on updating the rest of the administration UI. Progress can be tracked in the #3066007: Roadmap to stabilize Claro.

Some screenshots of Claro

Claro Content View

Claro Node Add

Claro Node Add Mobile

Claro Config Overview

Claro Config Block

Your feedback

  • Does this represent a positive change compared to the status quo (Seven)?
  • Does this introduce any major accessibility regressions?
  • Does this introduce any major usability regressions?

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

lauriii’s picture

Initial patch based on Claro 8.x-1.0-alpha5.

xjm’s picture

andrewmacpherson’s picture

  • Does this introduce any major accessibility regressions?
  • Does this introduce any major usability regressions?

I don't think the word "major" should be in here.

Should I review one of the tagged Claro releases, or just whatever is latest?

saschaeggi’s picture

@andrewmacpherson the latest tagged version would be Alpha 5 (released toady, 2019-09-06)

rainbreaw’s picture

@andrewmacpherson, in addition to removing the word "major" from those key questions, should we only be flagging concerns that would qualify as a "regression," or should we also bring up issues that might already exist in the status quo and persist here? (If this is even the case at all.)

xjm’s picture

I think we should identify any accessibility regressions that are not present in Seven. Any such would be major bugs with Claro and should block it being added to Standard. We can discuss which ones must block it being added as an alpha module theme if there are any that are really low-impact.

I tagged the wrong framework manager; there's no backend code here other than yaml files:

[ibnsina:drupal | Fri 16:14:52] $ git status | grep "php"
	new file:   core/themes/claro/images/core/cccccc/php-logo.svg

That said, as @lauriii posted the patch, having a backend framework manager look it over wouldn't hurt.

xjm’s picture

Here's the config and other yaml in the patch:

[ibnsina:drupal | Fri 16:18:46] $ git status | grep "yml"
	new file:   core/themes/claro/claro.breakpoints.yml
	new file:   core/themes/claro/claro.info.yml
	new file:   core/themes/claro/claro.libraries.yml
	new file:   core/themes/claro/config/install/block.block.claro_breadcrumbs.yml
	new file:   core/themes/claro/config/install/block.block.claro_content.yml
	new file:   core/themes/claro/config/install/block.block.claro_local_actions.yml
	new file:   core/themes/claro/config/install/block.block.claro_messages.yml
	new file:   core/themes/claro/config/install/block.block.claro_page_title.yml
	new file:   core/themes/claro/config/install/block.block.claro_primary_local_tasks.yml
	new file:   core/themes/claro/config/install/block.block.claro_secondary_local_tasks.yml
	new file:   core/themes/claro/config/install/claro.settings.yml
	new file:   core/themes/claro/config/optional/block.block.claro_help.yml
	new file:   core/themes/claro/config/schema/claro.schema.yml

It might be worth indicating in the IS if there are any changes to these vs. what's in Seven. If they're all identical we don't need to dig down on every one to the same extent.

Since we don't have a way of marking themes experimental AFAIK, we should mention that it is experimental in its description or something. We should consider whether to open an issue to allow themes to be marked as experimental the same way modules are.

A clean, accessible and powerful Drupal administration theme

Nit: missing serial comma after "accessible".

xjm’s picture

One of the things we need here from a release management perspective is the long-term roadmap. (See https://www.drupal.org/core/experimental#requirements .) In terms of technical debt and the maintenance of core, we'd likely want the longterm plan to be for Claro to eventually become Standard's admin theme (in some minor release of Drupal 9) and, ideally, Seven to be deprecated in Drupal 9 and moved to contrib for Drupal 10.

xjm’s picture

Another thing for the IS: How do the themes' regions compare to Seven's? This will affect a site's upgrade path if they choose to switch to Claro on their site, and so it would need to be documented in the CR/release note for the addition of Claro.

rainbreaw’s picture

Regarding accessibility: just getting started, but I love the focus style, clean use of icons, dividers, and space. I also love the nod to material design for maintaining user expectations of what design patterns mean. You have all clearly worked hard and put a lot of smart consideration into this! I have found some minor contrast issues, but would like to test a bit more before posting.

When I've found concerns to flag, would you like them here in a comment, or would you prefer new issues?

saschaeggi’s picture

@rainbreaw I'd say it might be easier if you create issues directly in the claro issue queue if you find something, thanks :)

webchick credited ckrina.

webchick credited huzooka.

webchick’s picture

Issue tags: +need screenshots

One more "needs" tag... it would greatly expedite the usability/product management review process, as well as generally excite people, to see some shots of what it looks like on major "90%" pages (e.g. node/add ; admin/content ; admin/config landing page). And/or a demo site if such a thing exists (simplytest.me has been very flaky lately :\).

GREAT work, everyone!! :D Adding a couple more people to commit credit who I know have been heavily involved.

webchick’s picture

Category: Task » Feature request
Priority: Normal » Critical

Also, from a product management perspective anyway, this is a critical feature, not a normal task. :) It holds back Drupal's adoption, it leads to capability perception issues, and the work done on Claro is light-years beyond in terms of design + accessibility.

Adding some more folks to commit credit, per @saschaeggi (thanks!)

jibran’s picture

Not to kick the hornet's nest, did we consider moving from yarn to npm?

Can we use core/package.json instead of core/themes/claro/package.json and move all the theme specifc packages to core?

Same goes for all the build js scripts for es6, can we use core ones to remove the duplication?

fhaeberle’s picture

@jibran What kind of improvement would it be to pick npm over yarn? Until now, the JavaScript build step in the Claro theme has been the same with core so we actually try hard not to differ too much from the existing one. Discussions on that are welcome in the Claro issue queue and in the Drupal Slack channel #admin-ui.

saschaeggi’s picture

RobLoach’s picture

Thought I'd chip in a quick review...

  1. Yarn. Is there a reason why Yarn is needed here? npm has come to a point where it provides much of what yarn has. Lock files, scripts, etc, npm is much improved since.
  2. Duplicate code. A lot of the JavaScript looks very similar to what's already provided by Drupal. Is there away to reduce the amount of duplicate code? tabledrag, for example.
  3. Scripts. ./node_modules/.bin/postcss \"css/src\" --dir \"css/dist\" --base \"css/src\""... When running an npm script, npm adds "bin" to the PATH. This means you can omit " ./node_modules/.bin/" from all of the "scripts" in package.json. https://docs.npmjs.com/misc/scripts
  4. jQuery Context. var $passwordInput = $(context).find('input.js-password-field').once('password');
    You can use var $passwordInput = $('input.js-password-field', context).once('password'); https://api.jquery.com/jQuery/#jQuery1
  5. Claro is listed with an install base of 341, it's also listed in alpha. Is there a large need for a new theme that looks pretty similar to Seven? I understand the goals are to bring some visual update to the administration theme. Could we then update the visual appeal of Seven rather than introducing a new admin theme? Make Seven feel more like Claro.

Overall, I like the goals, and the initiative. Some of these could be addressed in the contributed theme space. Would like to see Drupal improve from its inclusion, and not just be a new theme.

Chi’s picture

Is anyone really happy with new design of admin theme?

Artusamak’s picture

I'm so eager to have this in. It will be a game changer. Refreshing the backend theme was so necessary! Thank you for the hard work that has been put in so far.
I'm a total fan of the new design! Can't wait to have it in officially!

andrewmacpherson’s picture

Re. #6

should we only be flagging concerns that would qualify as a "regression," or should we also bring up issues that might already exist in the status quo and persist here?

The latter, most definitely:

  • Just because a there is a WCAG failure in the Seven theme, does NOT mean we should allow Claro to repeat it, and say "oh well, at least it's not technically a regression compared with Seven". Claro is a chance to correct the errors of the past, without the messy problem of having to worry about refactoring code which is already in use on a million web sites.
  • Seven was created in the olden days of WCAG 2.0. Now we have WCAG 2.1, and I've encouraged the Claro and Umami teams to target the updated standard. The Umami team did very well, and the Claro designs have taken a lot of it into account too; the clarity of the focus style, for example.

@xjm re. #7:

I think we should identify any accessibility regressions that are not present in Seven. Any such would be major bugs with Claro and should block it being added to Standard.

Whoa, what's the corollary to that... do you mean we should accept accessibility bugs on the basis that Seven already suffers from them? Even up to the point of Claro being used in Standard profile? I really can't accept that as coming remotely close to respecting the accessibility gate. Seven already has issues filed for WCAG failures, and there isn't anyone working on them. If there's a strategic initiative team backing Claro, with active contributors, and it doesn't result in better accessibility than Seven, then what's the point of the accessibility gate?

We can triage them all, of course. Generally I use:

  1. Major, bug, must-have for WCAG level A.
  2. Normal, bug, should-have for WCAG level AA.
  3. Normal, feature request, could-have for WCAG level AAA.

]EDIT: This is a generaous triage plan, btw. The USA's Section 508 requires level AA, so if we were following that we would need to treat A and AA as must-haves.]

I sometimes use major/must-have for accessibility issues which touch multiple level AA criteria, or impact several groups of users, or are well-known problems not addressed by WCAG. (Things only get included as Success Criteria in WCAG if they are deemed to be testable, as they form the basis of audits and conformance claims.) Occasionally I've argued something at level AAA is a should-have, when it's easy to address. You can expect me to justify such cases. Prioritize for impact, as our values state.

Some accessibility issues may turn out to be problems with core modules. Those won't be blockers for Claro. In fact, it will be preferable to fix them in the relevant modules for all themes to benefit, rather than paper over them in Claro.

Also re. #7:

We can discuss which ones must block it being added as an alpha module theme if there are any that are really low-impact.

Case in point: today I tested Claro and found it has the same "use of color alone" WCAG level A failure as this one in Seven. #2958282: Links inside surrounding text fail WCAG Use-of-Color in Seven theme. I filed that a year ago, but never get time to work on a11y issues in Seven because the strategic initiatives keep wanting a11y review. However Claro is part of a strategic admin-UI initiative, with active contributor, so I don't want to see it repeat this serious mistake. I'll file an issue for it in Claro.

shaal’s picture

Drupal core is the source and baseline for endless future modules and websites.
I'd love to help fix accessibility issues in Claro.

I created a short link to view all Claro's accessibility issues that are not resolved yet -
https://is.gd/claro_a11y

(The full link is more difficult to remember)
https://www.drupal.org/project/issues/search?projects=Claro&status%5B0%5...

xjm’s picture

Thanks everyone!

Regarding yarn vs. npm, discussing or changing that is out of scope here since we'd want to use one or the other throughout core. I'd suggest filing a separate core issue for that proposal if we want to explore it. (Search first in case someone's already proposed it.)

Thanks @andrewmacpherson and @shaal for the accessibility feedback.

@andrewmacpherson regarding:

Whoa, what's the corollary to that... do you mean we should accept accessibility bugs on the basis that Seven already suffers from them? Even up to the point of Claro being used in Standard profile? I really can't accept that as coming remotely close to respecting the accessibility gate. Seven already has issues filed for WCAG failures, and there isn't anyone working on them. If there's a strategic initiative team backing Claro, with active contributors, and it doesn't result in better accessibility than Seven, then what's the point of the accessibility gate?

I'd frame this the other way around. I meant to emphasize that we absolutely don't want accessibility or usability regressions over Seven and any such regressions should be must-have blockers on the roadmap. That doesn't mean we won't have other must-haves, or should-haves, for issues that are also present in Seven.

I think we definitely hope and intend for Claro to have accessibility improvements over Seven. We just need to be careful not to block a theme that already is an improvement on even more improvements of the existing accessibility technical debt in core. It's at least important to differentiate between things that are regressions from Seven versus things that exist in Seven or because of other parts of core. That said, for non-regressions, we can follow accessibility maintainers' recommendations for which are critical, major, etc. for the theme to be marked stable.

For committing the theme as alpha (which does not have to pass core gates yet), I think the requirement should be to add all the accessibility issues that are identified to the roadmap according to @andrewmacpherson's triage. We can have a related issues section at the end that lists the accessibility issues that are being inherited from Seven or from some module, to keep those issues on the radar of the initiative contributors.

Thank you!

andrewmacpherson’s picture

@xjm - Thanks for the clarification.

BTW, I'm very impressed with the accessibility efforts the Claro team is making. It's usually been the case with experimental modules that accessibility is left until after it's been marked beta, which is too late IMO. We've seen more encouraging approaches with our experimental themes. The Umami and Claro teams have both included accessibility in their early design phases, before writing any code. As a result of this, I'm reasonably optimistic that we could add Claro to core very soon as an alpha-status experimental theme, assuming the same general process as the experimental modules currently follow.

I've made an assessment task for Windows high-contrast support in #3080100: Assess accessibility of Claro in High Contrast AKA forced colors mode. Over there I've sketched out what I think accessibility triage looks like from alpha to stable, and eventually enabled-by-default. I think this triage approach could be made generic, so sometime soon I'll propose an amendment to our experimental extensions process to apply a11y reviews at the alpha and beta stages. After all, our gates were defined before we had experimental modules or a six month release cycle.

xjm’s picture

Issue tags: -need screenshots +Needs issue summary update

Oh also, belated thanks for the screenshots!

@gabesullice and @zrpnr discovered that #3005403: Cannot delete or edit a block that is placed in a section of the layout_builder also affects Claro. That's an example of an issue present in Seven that we should also add as a stable blocker for Claro, because it's borderline-critical (it'd be critical if LB were in Standard, for example).

Let's get a stub roadmap added to the IS here so that we can start filling in alpha, beta, and release must/should/could-haves.

xjm’s picture

andrewmacpherson’s picture

Claro contains a lot of extra JS, via libraries-extend and libraries-override. There isn't much explanation for this in the JS or YAML comments, and the Claro issue summaries don't explain much either. My understanding is that Claro's scope is a visual refresh with no changes to functionality, so the number of new JS files surprised me.

I expect it will take a lot of effort to assess this for accessibility. I know of several core bug reports where well-intended JS improvements have introduced serious accessibility regressions, particularly for keyboard users. Unfortunately, we currently have very little automated functional accessibility testing in core. Claro overrides the entire tabledrag library (~1700 line), and that's one where we have experienced keyboard regressions before.

To help review this, can the Claro team provide descriptions of why each JS library has been extended/overridden? Issue links will do if these have already been described elsewhere. If there are good technical reasons why Claro needs it's own JS, that's fine. I just need some help to get a sense of how much has changed before tackling manual accessibility review.

I don't think that manual a11y testing of all the new JS has to be completed before adding Claro to core as an alpha, but I'd like to know the scope of the JS changes before then.

If the existing core JS libraries are inadequate, shouldn't these be addressed in core for all themes, not just Claro? If Claro (as a core theme) has lots of theme-specific fixes and improvements, that puts it in a very privileged position compared to other themes. A contrib admin theme would have to re-implement the same changes to have the same quality as Claro, which seems a bit unfair to me.

Update: I already found the explanation for the vertical-tabs.js override in #3023310-78: Vertical Tabs style update. These are things which should be fixed in core for all themes. I don't know if issues have been filed for that.

effulgentsia’s picture

As just one example in support of #32, the patch in #2 includes a claro.media_library.view.es6.js file that overrides the entirety of Drupal.behaviors.MediaLibrarySelectAll(). AFAICT, the only purpose of this override is to theme the checkbox added by that function, and it's nice that there's a @todo in that file pointing to #3024975: Add Drupal JavaScript theme function for checkbox, which will allow the behavior override to be removed. However, in the meantime, the behavior override hasn't kept up with commits made to the original behavior. For example, the ones made in #3020716: Add vertical tabs style menu to media library and #2981044: Unify the grid/table views of the media library.

Either as part of preparing answers to #32, or in the course of this issue's other reviews, it will be important for someone to compare all of the JS overrides to make sure they're only overriding what they intend to and not introducing regressions similar to the above.

webchick’s picture

@ckrina gave a great demo to the product managers + UX team on this week's Drupal UX call (you can view the recording here: https://www.youtube.com/watch?v=6ajKn79VZK4)

The call included several people who've been using Claro for a period of weeks/months, so were able to give feedback from longer-term usage as well, which was awesome! While there was discussion on various aspects, such as possibly making the color band/icon the error messages thicker to make them more obviously error messages, there was really only one blocking concern, which is Views UI currently looks like this (minus the video player chrome; sorry about that):

Normally horizontal tabs are overlapping advanced settings

While it is technically possible to still use the Views UI despite this weird overlapping problem, it doesn’t seem like we can really add the theme to core until that is fixed, since we're talking about an "80%" site builder UI here. However, once that’s cleared up, we’re good to go from a product manager/UX sign off. (In fact, the earlier we get it into core, the more relief we will feel, since that means more time for testing prior to the release, to uncover other problems e.g. with contrib integrations and whatnot.)

Since this bug (#3067386: AJAX rebuild of Views UI form re-renders parts of the form without using Form API, so additional hooks are needed in lieu of hook_form_view_edit_form_alter()) is already captured as a "must-have" beta-blocker in the Claro roadmap #3066007: Roadmap to stabilize Claro, and since it's very clearly either fixed or not fixed (vs. something that is a bit squishier and needs e.g. a design round), I think we can safely remove the "needs" product manager + usability review tags from this issue, so we can at least get the sign-off train started. (Choo-choo!)

webchick’s picture

Adding participants from the UX call to the issue credit thinger.

webchick’s picture

Oh, one other interesting tidbit from that call is I guess @jrockowitz has been testing Webform with Claro, which helps speak to the breadth of "admin doohickeys" it can cover. Amazing work, folks!

andypost’s picture

As I got views has no design yet #3066006: Convert Views UI to new design system

joachim’s picture

Claro looks fantastic visually, but on a 13" laptop screen, admin content listings don't show any actual items above the fold! There is too much white space unfortunately.

bnjmnm’s picture

@joachim filed this issue based on what what reported in #43. I have similar concerns. It's pretty difficult to get a sense of most pages on a 13" laptop, especially if there are messages present. I'm on the fence about this being something that must get added to the roadmap, but I'm finding Seven to be much easier to use on 13" even though Claro looks much better.

webchick’s picture

Had a discussion today with @lauriii, @xjm, and @effulgentsia about next steps here. Here's a run-down:

  • We need a thorough code review of the entire theme by someone outside the initiative, ideally by both a front-end person (focused on HTML/CSS/JS) as well as a core framework manager (focused on architectural/theme system things). The patch above is a bit outdated now, so the tip of the branch of the contrib project should be used for this review: https://git.drupalcode.org/project/claro
  • #3060153: Use PostCSS in core, initially only for Claro has some outstanding questions from @xjm that need addressing.
  • Validation from other designers/site builders/etc. outside the initiative. There actually has been quite a lot of this (folks have been running Claro in production, integrating it with their contrib modules e.g. Webform, etc.), but I think a lot of it has taken place in UX meetings where there was a single comment summarizing lots of peoples' feedback. More feedback on-issue, please. :)
  • @effulgentsia's comment in #33 regarding the JavaScript needs addressing. @lauri pointed out that a lot of the duplicate JS is due to needing to maintain compatibility with 8.7, even though 8.8 has fixed many of the limitations; one possible way to approach this is like JSON:API did, and create a second branch focused specifically on core inclusion that removes the duplication.
  • Speaking of unaddressed comments, someone needs to go through ALL of the comments on this issue and ensure issues have been filed for them, or they've been otherwise addressed.
  • The accessibility MoSCoW needs to be buttoned up.
  • We don’t have a [technical] way to mark a theme as experimental right now. This needs an issue to discuss how we want to do that. (One idea to document in said issue is @effulgentsia suggested maybe solving the long-standing issue of #474684: Allow themes to declare dependencies on modules and then having the theme require a helper module that carries the experimental warning.)
  • #3066007: Roadmap to stabilize Claro has a good initial stab at a roadmap, but we need a more fleshed out one along the lines of #2834729: [META] Roadmap to stabilize Media Library that includes a full set of sub-issues and covers what specifically we're doing prior to commit, post-commit, beta, stable, etc.

I think that was everything. There is limited time in which to do all of this (8.8 code freeze is October 11), so if any of the ~60 followers here can help with one or more of these, that would be amazing!!

webchick’s picture

Added an issue for the experimental theme stuff at #3084069: Allow marking themes as experimental.

shimpy’s picture

Hii @webchick

As you had mention in #45 bullet 5 I had went through all the comments and create 2 news issues & added 3 other issues that were not there in the #3066007: Roadmap to stabilize Claro.

I have created one documentation page for Claro as administration theme:
https://www.drupal.org/docs/8/core/themes/claro-theme#comment-13277343

Please add it to menu as well

I have also listed all the comments on this discussion with the associated issues in a google sheet at:
https://docs.google.com/spreadsheets/d/199L3CZ1d4iBWZFTsmyDUUbufVUPr0ycq...

Thanks

shimpy’s picture

FileSize
100.27 KB
103.77 KB

@webchick

As you have mentioned in #15 I have worked on creating a demosite for claro theme setup as administartion theme.
i am sharing the domain
clarotheme.betabasket.net
For access please ping me in slack for admin credentials

Also attaching some screenshots for Seven as admin theme and Claro as admin theme.

Wim Leers’s picture

Status: Needs review » Needs work

@lauriii asked me to do a technical review of the theme itself, since that had not yet really happened. Which I guess is what the first bullet in #45 was about 🙂:

We need a thorough code review of the entire theme by someone outside the initiative, ideally by both a front-end person (focused on HTML/CSS/JS) as well as a core framework manager (focused on architectural/theme system things).

I'm neither a core framework manager nor a hard-core front-end person. But I am somewhere in between: I've done lots of work in the front end in the past, and I'm a de facto co-maintainer of the asset library system, render system, theme system and JavaScript. I think I was able to identify a bunch of things that would make a next review go much smoother 😄

  1. #8:

    It might be worth indicating in the IS if there are any changes to these vs. what's in Seven. If they're all identical we don't need to dig down on every one to the same extent.

    +1 — for that, let's use git diff -C -C — this will make git attempt really hard to record duplicates as "git copies" or duplicates with minor changes as "git copies plus diff". That'll make this 100x easier to assess.

  2. #23.2 WRT duplicate JS
    and
    #45's @effulgentsia's comment in #33 regarding the JavaScript needs addressing.

    The git diff -C -C suggestion I made in the previous point would help with that 🙂

  3. --- /dev/null
    +++ b/core/themes/claro/.eslintignore
    
    --- /dev/null
    +++ b/core/themes/claro/.eslintrc.json
    
    --- /dev/null
    +++ b/core/themes/claro/.eslintrc.passing.json
    
    --- /dev/null
    +++ b/core/themes/claro/.stylelintrc.json
    

    🤔 Other core themes do not have this. Why do we need it for Claro?

  4. +++ b/core/themes/claro/README.md
    @@ -0,0 +1,66 @@
    +## Working on Javascript
    ...
    +## Working on CSS
    

    🤔 Once #3060153: Use PostCSS in core, initially only for Claro lands, we can massively simplify this README, or even remove it.

  5. +++ b/core/themes/claro/claro.breakpoints.yml
    @@ -0,0 +1,12 @@
    +  mediaQuery: '(min-width: 0em)'
    ...
    +  mediaQuery: 'screen and (min-width: 40em)'
    

    🤔 Interesting — these are the exact same breakpoints as seven uses. Is that a coincidence or not? Should we document the rationale here?

  6. +++ b/core/themes/claro/claro.info.yml
    @@ -0,0 +1,171 @@
    +# As the UI of Drupal improves between minor versions, the mark up and assets
    

    🤓 s/mark up/markup/

  7. +++ b/core/themes/claro/claro.info.yml
    @@ -0,0 +1,171 @@
    +# version: VERSION
    

    🤓 This indeed had to be commented out while it's in contrib, but in core it needs to be uncommented.

  8. +++ b/core/themes/claro/claro.libraries.yml
    --- /dev/null
    +++ b/core/themes/claro/claro.theme
    

    😨 The sheer size of this file is frightening. seven.theme is already pretty bad with 188 LoC, but claro.theme is a whopping 1130 LoC! It could be significantly shorter if many of the discovered core bugs would be fixed. But even then, it'll be huge. But there isn't much we can do about that though.

    (In this patch it's <1000 LoC, but in the HEAD of the contrib module it's already grown to >1100 LoC.)

  9. +++ b/core/themes/claro/claro.theme
    @@ -0,0 +1,997 @@
    +function claro_preprocess_html(&$variables) {
    +  // If on a node add or edit page, add a node-layout class.
    +  $path_args = explode('/', \Drupal::request()->getPathInfo());
    +  if ($suggestions = theme_get_suggestions($path_args, 'page', '-')) {
    +    foreach ($suggestions as $suggestion) {
    +      if ($suggestion === 'page-node-edit' || strpos($suggestion, 'page-node-add') !== FALSE) {
    

    🤔 Why only page nodes and not other node types?

  10. +++ b/core/themes/claro/claro.theme
    @@ -0,0 +1,997 @@
    + * Makes node_add_list variables compatible with entity_add_list.
    ...
    +function claro_preprocess_node_add_list(&$variables) {
    ...
    + * Makes block_content_add_list variables compatible with entity_add_list.
    ...
    +function claro_preprocess_block_content_add_list(&$variables) {
    

    🤔 This should have a core issue so that we don't need to do these special snowflake alterations. Never mind, seven has the exact same ones 🤦‍♂️

  11. +++ b/core/themes/claro/claro.theme
    @@ -0,0 +1,997 @@
    +  // We require Modernizr for button styling.
    

    👎 Because? The reason is important to know: it allows us to remove this once Drupal only supports browsers that don't need this modernizr check!

  12. +++ b/core/themes/claro/css/dist/base/elements.css
    @@ -0,0 +1,196 @@
    +.cke_editable:not(.cke_editable_inline) {
    +  margin: 1em -webkit-calc(1em - 0.125rem);
    +  margin: 1em calc(1em - 0.125rem);
    +}
    

    🤔 Woah, a CKEditor-specific thing in the most "base" CSS there is? And then even CSS that is specifically targeting non-inline CKEditor instances? That seems like the wrong place, because this is now being loaded everywhere, including on the front end. This should only be loaded as part of the core/ckeditor asset library I think.

    #3023300: Text area style updates introduced this.

  13. +++ b/core/themes/claro/css/dist/base/print.css
    @@ -0,0 +1,88 @@
    +    display: table-header-group; /* h5bp.com/t */
    

    🤓 Is this … an old-skool comment hack? 😄

  14. +++ b/core/themes/claro/css/dist/base/typography.css
    @@ -0,0 +1,36 @@
    +* Reusable utility classes that apply vertical spacing consistency and in line
    

    🤓 s/in line/inline/

  15. +++ b/core/themes/claro/css/dist/base/typography.css
    --- /dev/null
    +++ b/core/themes/claro/css/dist/base/variables.css
    

    🤯 This file is 100% comments?!

  16. +++ b/core/themes/claro/css/dist/components/action-link.css
    @@ -0,0 +1,276 @@
    +  background-image: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' width='14px' height='16px' viewBox='0 0 14 16'%3E%3Cpath fill='%23545560' d='M13.9,2.9c-0.1-0.4-0.2-0.6-0.2-0.6c-0.1-0.4-0.4-0.4-0.8-0.5l-2.3-0.3c-0.3,0-0.3,0-0.4-0.3 C9.8,0.5,9.7,0,9.3,0H4.7C4.3,0,4.2,0.5,3.8,1.3C3.7,1.5,3.7,1.5,3.4,1.6L1.1,1.9C0.7,1.9,0.4,2,0.3,2.3c0,0-0.1,0.2-0.2,0.5 C0,3.4-0.1,3.3,0.4,3.3h13.2C14.1,3.3,14,3.4,13.9,2.9z M12.4,4.7H1.6c-0.7,0-0.8,0.1-0.7,0.6l0.8,10.1C1.8,15.9,1.8,16,2.5,16h9.1 c0.6,0,0.7-0.1,0.8-0.6l0.8-10.1C13.2,4.8,13.1,4.7,12.4,4.7z'/%3E%3C/svg%3E");
    

    🤓 Have these SVGs been optimized?

    🤔🤔🤔 More importantly: why are these inlined already? (These were done in #3036732: Action link component.) I'm sure that the build tools (#3060153: Use PostCSS in core, initially only for Claro) allow this to happen at build time? That'd make it much easier to inspect the SVGs, to apply SVG optimization tools, to encourage reuse, etc. It also allows us to decide whether we truly want to inline these as data: URIs, or whether it makes more sense to let them be loaded by HTTP requests. This particular CSS file is 24 KB ungzipped and unminified. That's … a lot. Most of that is due to these inlined SVG files (this CSS file is the single largest CSS file in Claro).

    In other words: for developer ergonomics and for web performance optimization reasons I think we should consider changing this.

  17. +++ b/core/themes/claro/css/dist/components/action-link.css
    @@ -0,0 +1,276 @@
    +/* Trash - danger */
    ...
    +/* Ex - danger */
    

    🤓 Should use em-dashes, shouldn't it? See https://www.chicagomanualofstyle.org/qanda/data/faq/topics/HyphensEnDash....

  18. +++ b/core/themes/claro/css/dist/components/ajax-progress.module.css
    @@ -0,0 +1,137 @@
    +.ajax-progress--fullscreen {
    

    😳 I was going to ask: Where do we use this?! but then I noticed that stable also contains this 🤯 Test coverage for this at \Drupal\FunctionalJavascriptTests\Ajax\ThrobberTest::testThemingThrobberElement. TIL.

  19. +++ b/core/themes/claro/css/dist/components/autocomplete-loading.module.css
    @@ -0,0 +1,81 @@
    + * With inline SVGs we can prevent a  browser get and repaint addressing the
    

    🤓

    a  browser get and repaint
    

    A space too many after a, and I think "browser get" means "a HTTP request"?

  20. +++ b/core/themes/claro/css/dist/components/autocomplete-loading.module.css
    @@ -0,0 +1,81 @@
    + * autocomplete input's background — until are sure that it will be pushed by
    ...
    + * The autocompleting (active) state's background-image is inlined because
    + * non-used CSS selectors are usually ignored; popular browsers don't download
    + * their 'url' references.
    ...
    + * repaint the background of autocomplete field. With the inlined background we
    + * can prevent an additional timeout caused by a new request/response pair.
    

    ❤️ Love this detailed rationale for inlining! That's what we should do in the other CSS file too. Then when browsers improve over time (or standardize on different behavior), we can easily reassess this instead of having to figure this out all over again! 👏

  21. +++ b/core/themes/claro/images/core/README.md
    @@ -0,0 +1,4 @@
    +## Purpose of this folder
    +Icons in this folder are copied from Drupal core. This folder with its content
    +should be removed before moving Claro to Drupal core. See
    +https://www.drupal.org/project/claro/issues/3045216 for details.
    

    🙏 This still needs to happen.

  22. +++ b/core/themes/claro/images/core/stable/views_ui/sprites.png
    @@ -0,0 +1,7 @@
    IHDR,���>gAMA��7��tEXtSoftwareAdobe ImageReadyq�e<�IDATx��LSW��{���b�C$�SF�t:0�Ȝh
    

    🙏 I'm 90% certain this raster image is not yet optimized.

  23. +++ b/core/themes/claro/images/noise-low.png
    --- /dev/null
    +++ b/core/themes/claro/images/shortcut/favstar-rtl.svg
    
    +++ b/core/themes/claro/images/spinner-rtl.gif
    --- /dev/null
    +++ b/core/themes/claro/images/src/arrow-down.svg
    
    +++ b/core/themes/claro/images/src/arrow-down.svg
    --- /dev/null
    +++ b/core/themes/claro/images/src/arrow-left.svg
    

    🙏 I'm 90% certain these SVGs are not yet optimized.

  24. +++ b/core/themes/claro/js/claro.details.es6.js
    @@ -0,0 +1,59 @@
    +   * Workaround for Firefox.
    +   *
    +   * Firefox applies the focus state only for keyboard navigation.
    

    🙏 Let's document to which Firefox versions this applies, so we can drop this in the future. Ideally, this also points to the Firefox bug tracker URL where this is being fixed.

  25. +++ b/core/themes/claro/js/claro.media_library.view.es6.js
    @@ -0,0 +1,48 @@
    + * @todo Remove after https://www.drupal.org/node/3024975 is in.
    
    +++ b/core/themes/claro/js/claro.tabledrag.es6.js
    @@ -0,0 +1,1715 @@
    + * @todo Remove after https://www.drupal.org/node/3024975 is in.
    

    👍 Great that we're being explicit about when this work-around/override can be removed!

    👎 Except that #3024975: Add Drupal JavaScript theme function for checkbox already landed 🤓

  26. +++ b/core/themes/claro/js/user.es6.js
    @@ -0,0 +1,311 @@
    + * @file
    + * Password confirm widget behaviors.
    + */
    ...
    +  Drupal.behaviors.password = {
    ...
    +  Drupal.evaluatePasswordStrength = (password, translate) => {
    ...
    +  Drupal.theme.passwordInputHelp = message =>
    

    👎 This should document why Claro needs to override something like this. I understand why Claro would override Drupal.theme.passwordInputHelp. I do not understand why it would override all JS code, including the password strength evaluation logic.

  27. --- /dev/null
    +++ b/core/themes/claro/package.json
    
    --- /dev/null
    +++ b/core/themes/claro/scripts/js/babel-es6-build.js
    

    🙂 I think this will go away once #3060153: Use PostCSS in core, initially only for Claro lands?

  28. +++ b/core/themes/claro/screenshot.png
    @@ -0,0 +1,259 @@
    +      <rdf:Description rdf:about=""
    +            xmlns:exif="http://ns.adobe.com/exif/1.0/"
    +            xmlns:tiff="http://ns.adobe.com/tiff/1.0/">
    

    🙏 130% certain that this raster image still needs to be optimized.

  29. +++ b/core/themes/claro/templates/admin/indentation.html.twig
    @@ -0,0 +1,36 @@
    +{% for i in 1..size if size > 0 %}<div class="js-indentation indentation">
    +  <svg
    ...
    +      class="tree__item tree__item-child-ltr tree__item-child-last-ltr tree__item-horizontal tree__item-horizontal-right"
    

    🤯🤯🤯🤯🤯🤯 Woah! This is a Twig template generating SVG!?! That's so cool. With class attributes so it can be targeted by CSS. And css/src/components/tabledrag.css is actually targeting it!

Wim Leers’s picture

droplet’s picture

To me, I'd add a single command for build & watch

for example:

`yarn watch` - this is running watcher for CSS & JS
`yarn build` - this is building the CSS & JS & Linting

+ "//": "'development is the default environment, and legacy is for transpiling the old jQuery codebase",

This is odd. Your development code is different than production.

lauriii’s picture

@droplet We've been working on adding build tools needed by Claro to core so that we don't have to ship Claro specific build tools in the theme. Your feedback might be more relevant in #3060153: Use PostCSS in core, initially only for Claro.

svettes’s picture

Based on comments above, here's a short summary of outstanding issues reviewed by Webchick, Svettes, and lauriii:
- Additional Framework Review for backend, front end and Release MGMT needed
- Input from accessibility review for roadmap needed
- Roadmap triage and prioritization based on feedback from reviews
- Patch to be posted to address comments in #49 point 2 and #33 (temporarily blocked on PostCSS issue)
- Issue to be resolved in comment #23 point 4 now added to issue #3086271
- Provide information about why a new theme was introduced (Claro) instead of updating the previous one (Seven)

Other notes:
- yml / diff between the two (seven / claro) are the same.
- wim’s comment #49 (points 1-4, 7, 13, 15, 21, 27), will be addressed in a patch that is coming shortly.
- Comment #2 will be addressed by the PostCSS patch
- https://www.drupal.org/project/drupal/issues/3084069 needs a BE dev to work on it

webchick’s picture

Provide information about why a new theme was introduced (Claro) instead of updating the previous one (Seven)

Product management definitely proposed to JUST do an incremental design over Seven in the original “spec” for this when the initiative was first announced: #2957457: Proposal: Modernize the look/feel of the Seven admin theme

However, NOT doing incremental patches over Seven (I'm not sure who on the implementing team ultimately made that call) has the following advantages:

1) The last time we tried a redesign of Seven https://groups.drupal.org/node/283223 we DID do it as incremental patches, only about 50-60% of which actually got committed, and it left the theme in a half-designed state. (Part of the rationale for the current redesign.)

2) This way, we can make incremental progress on the new design without “moving anyone’s cheese” until the thing is actually done. (As opposed to each release, AHHH! the whitespace + fonts get tweaked, then AHHH! the fielfield designs, etc.)

3) It’s really easy to get a picture of how "done" everything is at at any given time, because you enable the theme, and there you go. As opposed to questioning whether something looks off because of a bug in the "new" design or the "old" design, etc.

webchick’s picture

Also, HUGE thanks to @shimpy and @svettes for going through the issues and cataloguing what feedback was outstanding. (With cross-references and everything, @shimpy!! :O https://docs.google.com/spreadsheets/d/199L3CZ1d4iBWZFTsmyDUUbufVUPr0ycq...) Updating the issue credit accordingly.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.04 MB
677.42 KB

Here's a new patch using the core PostCSS compilation 🚀 This addresses feedback #49 1-4 and 7.

I also attached a version of the patch without the compiled CSS to make it easier to do code review.

saschaeggi’s picture

It's awesome to see how much progress we already made :) <3

andrewmacpherson’s picture

tl;dr - This is my accessibility maintainer sign-off for adding Claro to core as an experimental ALPHA theme. It's a yes from me.

I've been reviewing the theme (in a very roundabout way) for the last few weeks. Today I discussed it in Slack with @webchick, who asked:

It sounds like @andrewmacpherson is asking less for specific issues to block the alpha, but more a requirement for alpha that the theme as a whole to be given a first-pass review for the types of issues therein, and that these are catalogued in the Claro queue. [...]

It further sounded like in order to complete this "first pass," one of the things the a11y team requires is a run-down of the JS overrides in Claro and what has specifically changed (ideally, with pointers off to core issues where these were bug fixes in order to ensure they’re fixed properly upstream for all themes).

  1. Is that a correct understanding?
  2. Are you still waiting on that JS run-down from the Claro team?
  3. In either case, where is this kind of “first pass” review currently at, and what can we do to help remove impediments?

My answers (tidied up from the Slack conversation):

  1. Yes. For adding an alpha to core, I'm mostly looking at a first-pass a11y review. The team did a lot of thinking about a11y before even starting the code implementation. Focus styles, colour contrast, use of colour, and error styling are all well addressed. There are ongoing discussions about sizing and spacing, and work has begun on making it robust enough for Windows high-contrast mode.
  2. The JS run-down was the main concern, from comment #32 in this issue. Since then, I've been reading around a bunch of Claro issues and have a somewhat better idea of that now.
    • In many cases it's for want of better Drupal.theme.FOO functions. I've seen various TODOs in the Claro JS, and some of the upstream core issues they've opened. I think these theme function overrides are an understandable concern, and it looks like the plan is to reduce the amount of Claro-specific JS eventually.
    • BUT... the other thing I needed a better picture of, is whether there are any JS changes beyond those Drupal.theme.FOO issues. For instance, in #3023310: Vertical Tabs style update there was some discussion of whether to update vertical tabs to follow a more modern pattern from the WAI-ARIA authoring practices, plus other potential bugs in the current version. Eventually they shelved these as out of scope for Claro; they should be done in core for all themes. This is the right direction IMO. Changes of this nature would present the main risk of accessibility regressions and/or inconsistency.
    • Chatting to @lauriii in Slack, I gather that the intention is to avoid significant behaviour changes compared to core JS, and to greatly reduce the amount of Claro-specific JS. It'll be interesting to look at beta targets around those. I've also learned about what's going on in the 2.x branch, and #3084579: Claro Core inclusion that removes the duplicate JS.
    • So, I am now greatly reassured about the "Wow... how much javascript?!" situation, and #32 can be considered done from my viewpoint.

  3. Not so much else. Basically it was just the JS overview. I have a much better idea of the current state than I did a fortnight ago. I have some scribbled notes about some issues I want to file, but they are not alpha blockers.

Broadly speaking, I'm happy with this going in as an experimental alpha theme.

andrewmacpherson’s picture

@lauriii has just clarified that the Claro team is aiming to get to beta stage in 8.8.0 - a fact which was lost on me. We chatted about what a beta-stage a11y review looks like.

The review in #58 was for alpha, so I'm adding the a11y review tag back if this issue is actually about adding it as beta.

What I'm looking for is that we get a clearer WCAG conformance picture as we progress from alpha to beta....

  • For alpha, I was looking for a first-pass a11y review. We've established that colour, focus, input states, error styles, tables, headings and labels are all looking generally good.
  • For beta, I'm looking to get a more detailed level of review, against more WCAG success criteria. In particular how robust it is when users employ OS/browser preferences. The idea for beta is knowing what still needs fixing and how to do it. WCAG criteria could be beta blockers if they are unknowns, and/or we don't know how to fix them.
  • Issues don't have to be fixed until we want to mark it as stable.

BTW, Claro and Umami were way ahead of the experimental modules in this regard. Those have typically left a11y plans until after beta. That's the practice that we need to change in the project as a whole.

WCAG success criteria we still need to look at: text resize, text spacing, and reflow. Also what happens when users prevent author-specified fonts.

Good news is that screenshot evidence will do for a lot of it. I'm updating the roadmap with new issues to review this stuff, and to organize screenshot evidence. We can probably run some of it like "please do these things to your browser, and get before/after screenshots". Then there's a reduced workload for the a11y maintainers.

effulgentsia’s picture

#7 already removed the "Needs framework manager review" tag, but commented with:

That said, as @lauriii posted the patch, having a backend framework manager look it over wouldn't hurt.

FYI: I'm part way through reviewing it. I'm far enough along with it that if this were still tagged with "Needs framework manager review", and if that applied only to the initial alpha-stability commit to core, then I would remove that tag at this point.

With respect to a review for beta stability, nothing has yet jumped out at me other than what's already captured in #3066007: Roadmap to stabilize Claro, plus the need for at least some minimal level of test coverage per #3066007-39: Roadmap to stabilize Claro. I discussed that today with @alexpott and @lauriii, and @alexpott suggested that a good level of test coverage for beta would be a test that visits a set of pages, that combined include all of Claro's JS. That way if there are any JS errors triggered merely by visiting a page with that JS on it, we'd catch that. @lauriii said he'll open an issue for that. If we're able to get that in by alpha, then great, but I personally don't consider it a must-have requirement until beta.

webchick’s picture

#59/#60: A conversation about gates with respect to experimental projects came up with core committers this evening, and it was pointed out that the documentation at https://www.drupal.org/core/experimental#requirements specifically states that core gate passage is not required until the "stable" milestone. Proposing to change this policy to require core gate adherence (or a subset thereof) before that milestone (e.g. at beta time) is not a bad idea, and definitely worthy of discussion, but we'd need to do that in its own dedicated policy issue and aim it at the 9.1 release, not 1 week before 8.8 alpha. :)

What we should do for Claro's beta is make sure there are either issues or "issue descriptions" filed in the roadmap issue for each of the things that needs to get done to satisfy the gates, as @andrewmacpherson has done with e.g. "Assess conformance with WCAG SC 1.4.4 "Resize text". NEEDS CHILD ISSUE to capture test process and evidence." (And some guidance on how to actually do those assessments would be welcome, but I think you said that's forthcoming.)

xjm’s picture

Issue tags: +Needs followup

Thanks @webchick.

@lauriii also suggested that we still treat @andrewmacpherson's suggestions for beta as should-haves; i.e., we will still prioritize them first if we have time once the must-haves are done. That way we can still get the info the accessibility reviewers need from the high-contrast assessment etc. early, so long as the technical requirements for beta are already able to be met. And then if we didn't get to it, it would still be the first must-have on the list for stable.

Sorry for the confusion! I tagged "Needs followup" so we can have a discussion about potential additional beta requirements, if these will help the accessibility maintainers in the future.

andrewmacpherson’s picture

#61 - #62:
I am indeed going to propose a change to how core gates are applied to experimental extensions. The gates date from before we even had 6-monthly releases, or experimental extensions. Looking back over the original discussions about the accessibility gate, there was significant discussion about the importance of reviewing prototypes early, but it wasn't clear how/if that could fit with the release model at the time. It's encouraging that our experimental themes since D8.0.0 have all started addressing accessibility before writing code. Experimental modules have largely left it until after being marked as beta, and I think that's precisely because the gates currently don't apply until the stable stage. D9 may be an good time to update the experimental extension process, since we have the 8.9/9.0 special cleanup release where new features aren't the priority. For accessibility, I'm emphasizing early review, rather than early fixes. I imagine that other gates could have their own alpha/beta review approaches (e.g. performance budgets).

In the meantime, I am happy to treat the beta-stage a11y reviews as should-haves, as in #62.

I'll update the roadmap status now, and file the gate proposal issue soon.

andrewmacpherson’s picture

Hang on, I agreed too soon. Usability is a gate, too.

The roadmap currently lists must-haves for beta under "usability improvements" and "design improvements".

If we're not going to treat accessibilty reviews as must-have targets for beta, then I don't see how we can treat usability implementations as must-haves for beta. That sounds like one gate gets priority over another.

lauriii’s picture

#64: Both usability improvements and design improvements are listed only under stable must-haves. Maybe you were looking at the stable section of the list?

xjm’s picture

Issue tags: +Needs change record
DyanneNova’s picture

I've reviewed the CSS, twig templates, and am about halfway through the preprocess functions. The only beta blocking issue that I found is currently being worked on at #3067386: AJAX rebuild of Views UI form re-renders parts of the form without using Form API, so additional hooks are needed in lieu of hook_form_view_edit_form_alter().

xjm’s picture

@andrewmacpherson Thanks! I just double-checked and there are no usability or design improvements listed for beta, only stable. The beta blockers are the MVP feature-set, the required criteria for core inclusion, and one bug that was flagged as critical. So I think we're still on the same page and I updated the roadmap accordingly.

svettes’s picture

Status summary:

  • Claro Roadmap is updated based on review of issue queue
  • 8.8.0-Alpha1 framework reviews are nearly complete
  • Requirements for 8.8.0-Beta1 still need clarification


Risks related to the 8.8 release:

  • Alpha: If we do not meet Beta criteria we would need to remove Claro from 8.8.x before 8.8.0-alpha1. To avoid the additional work of taking it out of core, the mitigation strategy today is to add it to the 8.9.x branch so that if it meets the 8.8.0-beta1 requirements it can be backported into 8.8.x, and then put into the following alpha and beta releases of 8.8.
  • Beta: Clarity of requirement for beta and JS/d w reviews must be done next week to stay on track for release schedule.


Next steps:

  • Clarification on requirements for 8.8.0-Beta1 (goal: by 10/10)
  • Framework mgmt additional reviews (goal: by 10/11)
  • JS review asap (goal: by 10/11)
  • Views UI issue resolved asap (goal: by 10/11)
  • Commit Claro to 8.9.x on 10/11
  • Any work brought up by reviews is completed (goal: by 10/18)
  • Final RM Review (goal: by 10/18)
  • Drupalcon happens and we all go do stuff together at the sprints hopefully!
  • Commit Claro to 8.8.x by 11/1 (enabling it to be included in 8.8.0-alphax release and then in the 8.8.0-beta1 release the following week)
xjm’s picture

Requirements for 8.8.0-Beta1 still need clarification

What is unclear?

lauriii’s picture

Here's an updated patch. This adds test coverage and has some extra clean up. This should also pass eslint and stylelint.

Wim Leers’s picture

Initial draft change record created: https://www.drupal.org/node/3087157

lauriii’s picture

This actually adds the Claro test coverage. That was accidentally missing from #71.

lauriii’s picture

justafish’s picture

Status: Needs review » Needs work

Nothing particularly grim is jumping out at me from the JS side (other than us being stuck with this old-style JS of course!).

Just a few minor things:

  • Some files are prefixed with claro. and others aren't, let's just go with 1 convention (my preference would be without the claro. prefix)
  • Split the contents of Claro theme.es6.js into checkbox.es6.js and dropbutton.es6.js so it's obvious which things these are overriding

This table was helpful reviewing this, maybe it could be useful to put into the documentation? https://docs.google.com/document/d/1nIZpXgCV8-AnRExADiKN85dKHWshI_wzqz1z...

and just in case this is useful for anyone else trying to review this: https://github.com/justafish/drupal/pull/4/files

lauriii’s picture

I opened #3087187: Remove claro. prefix from JavaScript files and #3087188: Split the contents of Claro theme.es6.js into checkbox.es6.js and dropbutton.es6.js in the Claro issue queue to address #75.

I confirmed with @justafish on Slack that the "needs JavaScript review" tag can be removed given that we've opened issues in the Claro issue queue to address #75.

lauriii’s picture

Status: Needs work » Needs review
FileSize
653.26 KB
1.01 MB

This should fix the tests and address #75.

andrewmacpherson’s picture

#65 and #68:
Thanks for double-checking and correcting me, @lauriii and @xjm. I was probably in a defensive, parochial mood when I wrote #64.

I've started fleshing out some of the manual a11y survey plans, for example #3087225: [META] Assess Claro's conformance with WCAG Text spacing, Text resize, and Reflow..

Following #68, I'm attempting to find achievable approaches for first-pass surveys prior to beta. So at beta stage, we can say some progress has been made towards these.

Full-page screen captures from the Styleguide and Error style modules would provide good overviews for these robustness WCAG criteria. More details in the survey issues themselves.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

catch’s picture

OK I've had a look over the roadmap several times, but mea culpa I have not actually reviewed Claro.

From a release management standpoint though, #3066007: Roadmap to stabilize Claro looks in a good state to commit to 8.9.x as alpha, and if the two remaining beta blockers on that issue are fixed (and no new ones found, the shortcut one was a recent find), backport to 8.8.x as beta.

There's a very long list of must and should haves for a stable release but it's much better to have a long documented list, than a short list with lots of things missing/hidden/undiscovered, so that's encouraging.

xjm’s picture

+1 for #80. I've also not reviewed the codebase itself, but I've been watching the issues that have come out of the review process, and I think the identified beta blockers capture what's needed to ship this safely as beta in a tagged release. Thanks everyone for your work on this patch and roadmap!

lauriii’s picture

ckrina’s picture

Status: Needs review » Reviewed & tested by the community

We just discussed with @lauriii and with the last changes we can mark this RTBC while we wait for the last frontend framework manager review.

Thanks everybody that has invested time on this and has made it possible to start the next step for Claro: getting into core! From the design team to the implementation, the accessibility reviews or previous UX research and everybody that has helped keeping the issue queue up to date. Great team work!

It means that from now on the goal is to get Claro into a Stable release in the next months, so it means we have a huge amount of work in front of us. We will need to prioritize accessibility and finish all the design work missing, and continue working on the Roadmap and the feedback that hopefully will come with Claro in core.

Thanks all! 🙌

DyanneNova’s picture

I've reviewed all of the theme preprocess functions now and didn't see any new issues. I agree that Claro looks ready for alpha, and ready for beta once the known beta blocking issues are fixed.

lauriii credited AntoineH.

lauriii credited finnsky.

lauriii credited lot007.

lauriii credited pzajacz.

lauriii credited quiron.

lauriii’s picture

Thank you all for the reviews!

I'm going to untag this from the frontend framework manager review since I've been participating in the development process of the theme and I've essentially reviewed every commit that was made in the contrib project. This has been reviewed from the framework manager's point of view by @effulgentsia, and on top of that @DyanneNova has reviewed the CSS, templates and preprocess functions in preparation for beta release. I think this should be enough validation to remove the tag.

The next step is for a committer to review the patch to be committed to 8.9.x. In the meanwhile, we will work on solving #3067386: AJAX rebuild of Views UI form re-renders parts of the form without using Form API, so additional hooks are needed in lieu of hook_form_view_edit_form_alter() which is the final beta-blocking issue. After all the beta-blocking issues have been resolved, we can start preparing a backport to 8.8.x.

That means I'm +1 to the RTBC.

lauriii credited L2G2.

lauriii credited anevins.

lauriii credited evankay.

lauriii credited kiboman.

lauriii credited leymannx.

lauriii credited mherchel.

lauriii credited mlncn.

lauriii credited rafuel92.

lauriii credited thamas.

lauriii credited volkerk.

lauriii’s picture

lauriii credited DuneBL.

lauriii credited Eli-T.

lauriii credited alexpott.

lauriii credited idebr.

lauriii credited pivica.

lauriii credited zrpnr.

lauriii’s picture

lauriii credited ccasals.

lauriii credited hampercm.

lauriii credited if-jds.

lauriii credited imalabya.

lauriii credited jhedstrom.

lauriii credited mandclu.

lauriii credited modulist.

lauriii credited nod_.

lauriii credited pranav73.

lauriii’s picture

lauriii credited maliknaik.

lauriii’s picture

All credits should be now done. I added credit for everyone who had received at least one credit in the Claro project.

shimpy’s picture

@lauriii
Hii do i deserve some credits for #47 and #48??

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review

When trying to commit this to the 9.0.x branch (which is where it needs to go first and then be cherry picked from there), I'm getting Stylelint errors from the pre-commit git hook in https://github.com/alexpott/d8githooks.

A bunch of CSS files are erroring. Here's an example of one of them:

core/themes/claro/css/src/base/elements.css
 57:14  ✖  Expected newline after ":" with a multi-line declaration   declaration-colon-newline-after
 58:1   ✖  Expected indentation of 4 spaces                           indentation                    
 58:1   ✖  Unexpected whitespace before ","                           value-list-comma-space-before  
 59:1   ✖  Expected indentation of 4 spaces                           indentation                    
 60:1   ✖  Expected indentation of 4 spaces                           indentation                    
 60:1   ✖  Unexpected whitespace before ","                           value-list-comma-space-before  
 61:1   ✖  Expected indentation of 4 spaces                           indentation                    
 62:1   ✖  Expected indentation of 4 spaces                           indentation                    
 62:1   ✖  Unexpected whitespace before ","                           value-list-comma-space-before  
 63:1   ✖  Expected indentation of 4 spaces                           indentation                    
 64:1   ✖  Expected indentation of 4 spaces                           indentation                    
 64:1   ✖  Unexpected whitespace before ","                           value-list-comma-space-before  
 65:1   ✖  Expected indentation of 4 spaces                           indentation                    
 66:1   ✖  Expected indentation of 4 spaces                           indentation                    
 66:1   ✖  Unexpected whitespace before ","                           value-list-comma-space-before  
 67:1   ✖  Expected indentation of 4 spaces                           indentation                    
 68:1   ✖  Expected indentation of 4 spaces                           indentation                    
 68:1   ✖  Unexpected whitespace before ","                           value-list-comma-space-before  
 69:1   ✖  Expected indentation of 4 spaces                           indentation                    
 70:1   ✖  Expected indentation of 4 spaces                           indentation                    
 70:1   ✖  Unexpected whitespace before ","                           value-list-comma-space-before  
 71:1   ✖  Expected indentation of 4 spaces                           indentation                    
 72:1   ✖  Expected indentation of 4 spaces                           indentation                    
 72:1   ✖  Unexpected whitespace before ","                           value-list-comma-space-before  
 73:1   ✖  Expected indentation of 4 spaces                           indentation

I'm also getting several of the following errors for JS files:

yarn run v0.27.5
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js "--check" "--file" "/.../core/themes/claro/js/mobile.install.es6.js"
[22:56:13] '/.../core/themes/claro/js/mobile.install.es6.js' is being checked.
[22:56:13] '/.../core/themes/claro/js/mobile.install.es6.js' is not updated.
error Command failed with exit code 1.

Any ideas on what's wrong?

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

#139: Only people who haven't commented on this issue are highlighted as a separate comment. You've been already added to the list of people being credited 👏 Thank you for your contributions!

#140: It seems like the git hooks are trying to lint the PostCSS generated CSS files. I opened a PR in Github to solve this: https://github.com/alexpott/d8githooks/pull/26. When running yarn lint:css there shouldn't be any linting errors.

shimpy’s picture

Thanks @lauriii

AaronMcHale’s picture

I said it on a recent UX call but I'll say it here, I've been using Claro for a while now and I've really enjoyed using it! Everyone involved in Claro has done a fantastic job, I'm really excited to see this make it into core and I think it's going to be a fantastic addition to Drupal. Everyone should be extremely proud of what's been accomplished here 🎉

effulgentsia’s picture

Looks like there's feedback on the PR linked to in #141. I'll keep an eye on it.

Meanwhile, as far as #140's JS error, it looks to me like it's due to a possible error (or at least difference) in how the Claro ES6 to ES5 transpilations were done.

Here's an example of core's Drupal.theme.ajaxProgressThrobber ES6 function:

Drupal.theme.ajaxProgressThrobber = message => {
    // Build markup without adding extra white space since it affects rendering.
    const messageMarkup =
      typeof message === 'string'
        ? Drupal.theme('ajaxProgressMessage', message)
        : '';
    const throbber = '<div class="throbber">&nbsp;</div>';

    return `<div class="ajax-progress ajax-progress-throbber">${throbber}${messageMarkup}</div>`;
  };

And here's Claro's:

Drupal.theme.ajaxProgressThrobber = message => {
    // Build markup without adding extra white space since it affects rendering.
    const messageMarkup =
      typeof message === 'string'
        ? Drupal.theme('ajaxProgressMessage', message)
        : '';
    const throbber = '<div class="ajax-progress__throbber">&nbsp;</div>';

    return `<div class="ajax-progress ajax-progress--throbber">${throbber}${messageMarkup}</div>`;
  };

Identical except for the values of the class attributes.

However, here's how that function was transpiled to ES5 in core:

Drupal.theme.ajaxProgressThrobber = function (message) {
    var messageMarkup = typeof message === 'string' ? Drupal.theme('ajaxProgressMessage', message) : '';
    var throbber = '<div class="throbber">&nbsp;</div>';

    return '<div class="ajax-progress ajax-progress-throbber">' + throbber + messageMarkup + '</div>';
  };

And here's how it was transpiled to ES5 in the Claro patch:

Drupal.theme.ajaxProgressThrobber = function (message) {
    var messageMarkup = typeof message === "string" ? Drupal.theme("ajaxProgressMessage", message) : "";
    var throbber = '<div class="ajax-progress__throbber">&nbsp;</div>';

    return "<div class=\"ajax-progress ajax-progress--throbber\">" + throbber + messageMarkup + "</div>";
  };

Looks like the Claro transpilation prefers double quotes to single quotes. I think when my pre-commit hooks runs, it uses core's rule of single quotes and then detects that as a difference to what's in the Claro patch and errors out.

Or, did core's rule change to double quotes at some point, and am I just running an obsolete version of something?

droplet’s picture

#144

The patch is outdated.

  --font-size-h1: 2.027rem; /* ~32px */
  --font-size-h2: 1.802rem; /* ~29px */
  --font-size-h3: 1.602rem; /* ~26px */
  --font-size-h4: 1.424rem; /* ~23px */
  --font-size-h5: 1.266rem; /* ~20px */
  --font-size-h6: 1.125rem; /* 18px */
  --font-size-s: 0.889rem; /* ~14px */
  --font-size-xs: 0.79rem; /* ~13px */
  --font-size-xxs: 0.702rem; /* ~11px */

I'm interested in how these values coming up. If that was a @3x design, it's a common issue the designer used a non "multiples of 3" value. I think it can be tidy up. Cross-browser handling the sub-pixel differently.

calc(17rem / 16);
many codes of these styles. It doesn't take advantage of calc, custom var & REM unit.

If you want to scale the base 2px to 18px in :root. Everywhere is broken. There's no much different than use PX.

lauriii’s picture

I was missing a JavaScript compilation step from the script that generated the patch. JavaScript files in this patch are generated with the core tooling and I checked that it addresses #144.

There was also a wrong filetype for PostCSS files in .stylelintignore. This has been also addressed in this patch.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 146: 3079738-146.patch, failed testing. View results

xjm’s picture

Status: Needs work » Reviewed & tested by the community

  • effulgentsia committed 1786bc1 on 9.0.x
    Issue #3079738 by lauriii, saschaeggi, webchick, xjm, andrewmacpherson,...

  • effulgentsia committed 0fe72eb on 8.9.x
    Issue #3079738 by lauriii, saschaeggi, webchick, xjm, andrewmacpherson,...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Amazing work, everyone!!! Pushed to 9.0.x and 8.9.x.

Not pushing this to 8.8.x until all the must-haves for beta stability are complete. For that same reason, also leaving the CR unpublished for now.

I think it makes sense to mark this issue Fixed for 8.9.x, and then doing the 8.8.x commit in a separate issue.

jibran’s picture

No, MAINTAINERS.txt entry.

lauriii’s picture

No, MAINTAINERS.txt entry.

I confirmed with @catch that it is not a requirement for adding new experimental modules or themes. We will be adding one later. 😇

  • alexpott committed 5940274 on 9.0.x
    Issue #3079738 follow-up: Update composer.lock for Claro theme
    
ivnish’s picture

del

effulgentsia’s picture

Not pushing this to 8.8.x until all the must-haves for beta stability are complete.

They're complete now, so here's the issue for cherry picking to 8.8: #3088437: Cherry pick Claro to Drupal 8.8.. The one thing missing from there is the composer.lock update of #155. Is there a composer command to run to get that change into the lock file? If you know, please comment in that issue.

  • effulgentsia committed 3c80c6a on 8.8.x
    Issue #3079738 by lauriii, saschaeggi, webchick, xjm, andrewmacpherson,...
effulgentsia’s picture

Version: 8.9.x-dev » 8.8.x-dev

Pushed to 8.8.x and published the CR!

Status: Fixed » Closed (fixed)

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

shimpy’s picture