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
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
Comment | File | Size | Author |
---|---|---|---|
#146 | 3079738-146.patch | 1.03 MB | lauriii |
#82 | 3079738-82.patch | 1.03 MB | lauriii |
#82 | 3079738-82-source-only.patch | 667.04 KB | lauriii |
#77 | 3079738-77.patch | 1.01 MB | lauriii |
#77 | 3079738-77-source-only.patch | 653.26 KB | lauriii |
Comments
Comment #2
lauriiiInitial patch based on Claro 8.x-1.0-alpha5.
Comment #3
xjmYay!
All the tags.
Comment #4
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedI don't think the word "major" should be in here.
Should I review one of the tagged Claro releases, or just whatever is latest?
Comment #5
saschaeggi@andrewmacpherson the latest tagged version would be Alpha 5 (released toady, 2019-09-06)
Comment #6
rainbreaw CreditAttribution: rainbreaw as a volunteer and commented@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.)
Comment #7
xjmI 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
moduletheme 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:
That said, as @lauriii posted the patch, having a backend framework manager look it over wouldn't hurt.
Comment #8
xjmHere's the config and other yaml in the patch:
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".
Comment #9
xjmOne 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.
Comment #10
xjmAnother 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.
Comment #11
rainbreaw CreditAttribution: rainbreaw as a volunteer and commentedRegarding 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?
Comment #12
saschaeggi@rainbreaw I'd say it might be easier if you create issues directly in the claro issue queue if you find something, thanks :)
Comment #15
webchickOne 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.
Comment #19
webchickAlso, 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!)
Comment #20
jibranNot to kick the hornet's nest, did we consider moving from yarn to npm?
Can we use
core/package.json
instead ofcore/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?
Comment #21
fhaeberle@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.
Comment #22
saschaeggiComment #23
RobLoachThought I'd chip in a quick review...
./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/scriptsvar $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/#jQuery1Overall, 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.
Comment #24
Chi CreditAttribution: Chi commentedIs anyone really happy with new design of admin theme?
Comment #25
ArtusamakI'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!
Comment #26
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedRe. #6
The latter, most definitely:
@xjm re. #7:
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:
]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:
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.
Comment #27
shaalDrupal 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...
Comment #28
xjmThanks 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:
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!
Comment #29
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commented@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.
Comment #30
xjmOh 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.
Comment #31
xjmComment #32
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedClaro 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.
Comment #33
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAs just one example in support of #32, the patch in #2 includes a
claro.media_library.view.es6.js
file that overrides the entirety ofDrupal.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.
Comment #34
webchick@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):
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!)
Comment #40
webchickAdding participants from the UX call to the issue credit thinger.
Comment #41
webchickOh, 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!
Comment #42
andypostAs I got views has no design yet #3066006: Convert Views UI to new design system
Comment #43
joachim CreditAttribution: joachim as a volunteer commentedClaro 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.
Comment #44
bnjmnm@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.
Comment #45
webchickHad a discussion today with @lauriii, @xjm, and @effulgentsia about next steps here. Here's a run-down:
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!!
Comment #46
webchickAdded an issue for the experimental theme stuff at #3084069: Allow marking themes as experimental.
Comment #47
shimpyHii @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
Comment #48
shimpy@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.
Comment #49
Wim Leers@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 🙂:
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 — 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.and
#45's
The
git diff -C -C
suggestion I made in the previous point would help with that 🙂🤔 Other core themes do not have this. Why do we need it for Claro?
🤔 Once #3060153: Use PostCSS in core, initially only for Claro lands, we can massively simplify this README, or even remove it.
🤔 Interesting — these are the exact same breakpoints as
seven
uses. Is that a coincidence or not? Should we document the rationale here?🤓 s/mark up/markup/
🤓 This indeed had to be commented out while it's in contrib, but in core it needs to be uncommented.
😨 The sheer size of this file is frightening.
seven.theme
is already pretty bad with 188 LoC, butclaro.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.)
🤔 Why only
page
nodes and not other node types?✅
🤔 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 🤦♂️👎 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!
🤔 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.
🤓 Is this … an old-skool comment hack? 😄
🤓 s/in line/inline/
🤯 This file is 100% comments?!
🤓 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.
🤓 Should use em-dashes, shouldn't it? See https://www.chicagomanualofstyle.org/qanda/data/faq/topics/HyphensEnDash....
😳 I was going to ask:
but then I noticed thatstable
also contains this 🤯 Test coverage for this at\Drupal\FunctionalJavascriptTests\Ajax\ThrobberTest::testThemingThrobberElement
. TIL.🤓
A space too many after
a
, and I think "browser get" means "a HTTP request"?❤️ 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! 👏
🙏 This still needs to happen.
🙏 I'm 90% certain this raster image is not yet optimized.
🙏 I'm 90% certain these SVGs are not yet optimized.
🙏 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.
👍 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 🤓
👎 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.🙂 I think this will go away once #3060153: Use PostCSS in core, initially only for Claro lands?
🙏 130% certain that this raster image still needs to be optimized.
🤯🤯🤯🤯🤯🤯 Woah! This is a Twig template generating SVG!?! That's so cool. With
class
attributes so it can be targeted by CSS. Andcss/src/components/tabledrag.css
is actually targeting it!Comment #50
Wim LeersComment #51
droplet CreditAttribution: droplet commentedTo 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
This is odd. Your development code is different than production.
Comment #52
lauriii@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.
Comment #53
svettes CreditAttribution: svettes at Acquia commentedBased 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
Comment #54
webchickProduct 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.
Comment #55
webchickAlso, 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.
Comment #56
lauriiiHere'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.
Comment #57
saschaeggiIt's awesome to see how much progress we already made :) <3
Comment #58
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedtl;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:
My answers (tidied up from the Slack conversation):
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.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.So, I am now greatly reassured about the "Wow... how much javascript?!" situation, and #32 can be considered done from my viewpoint.
Broadly speaking, I'm happy with this going in as an experimental alpha theme.
Comment #59
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commented@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....
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.
Comment #60
effulgentsia CreditAttribution: effulgentsia at Acquia commented#7 already removed the "Needs framework manager review" tag, but commented with:
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.
Comment #61
webchick#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.)
Comment #62
xjmThanks @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.
Comment #63
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commented#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.
Comment #64
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedHang 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.
Comment #65
lauriii#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?
Comment #66
xjmComment #67
DyanneNovaI'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().
Comment #68
xjm@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.
Comment #69
svettes CreditAttribution: svettes at Acquia commentedStatus summary:
Risks related to the 8.8 release:
Next steps:
Comment #70
xjmWhat is unclear?
Comment #71
lauriiiHere's an updated patch. This adds test coverage and has some extra clean up. This should also pass eslint and stylelint.
Comment #72
Wim LeersInitial draft change record created: https://www.drupal.org/node/3087157
Comment #73
lauriiiThis actually adds the Claro test coverage. That was accidentally missing from #71.
Comment #74
lauriiiComment #75
justafishNothing 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:
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
Comment #76
lauriiiI 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.
Comment #77
lauriiiThis should fix the tests and address #75.
Comment #78
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commented#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.
Comment #80
catchOK 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.
Comment #81
xjm+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!
Comment #82
lauriiiThis resolves #3087189: Resolve or work around claro.settings.yml implicit dependency on Shortcut which is the final alpha-blocking issue.
Comment #83
ckrinaWe 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! 🙌
Comment #84
DyanneNovaI'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.
Comment #95
lauriiiThank 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.
Comment #108
lauriiiComment #121
lauriiiComment #134
lauriiiComment #138
lauriiiAll credits should be now done. I added credit for everyone who had received at least one credit in the Claro project.
Comment #139
shimpy@lauriii
Hii do i deserve some credits for #47 and #48??
Comment #140
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWhen 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:
I'm also getting several of the following errors for JS files:
Any ideas on what's wrong?
Comment #141
lauriii#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.Comment #142
shimpyThanks @lauriii
Comment #143
AaronMcHaleI 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 🎉
Comment #144
effulgentsia CreditAttribution: effulgentsia at Acquia commentedLooks 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:And here's Claro's:
Identical except for the values of the
class
attributes.However, here's how that function was transpiled to ES5 in core:
And here's how it was transpiled to ES5 in the Claro patch:
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?
Comment #145
droplet CreditAttribution: droplet commented#144
The patch is outdated.
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.
Comment #146
lauriiiI 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.
Comment #147
lauriiiOpened follow-up for #145: #3087648: #3079738-145: Feedback from @droplet related to font-sizes.
Comment #149
xjmComment #152
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAmazing 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.
Comment #153
jibranNo, MAINTAINERS.txt entry.
Comment #154
lauriiiI confirmed with @catch that it is not a requirement for adding new experimental modules or themes. We will be adding one later. 😇
Comment #156
ivnish CreditAttribution: ivnish commenteddel
Comment #157
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThey'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.
Comment #159
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 8.8.x and published the CR!
Comment #161
shimpy