g4mbini → credited mherchel → .
Opened followup issues:
- 📌 Refactor “Get started” menu at desktop Postponed
- 🐛 Menus items should close on focus out (both at desktop and mobile) Postponed
- 🐛 Menu not accessible in forced colors mode Postponed
- 🐛 Mobile menu should have overlay when open Postponed
- 📌 Menus should close on ESC Postponed
mherchel → created an issue.
mherchel → created an issue.
mherchel → created an issue.
mherchel → created an issue.
mherchel → created an issue.
Thanks so much Putra! I'm sure we'll run into each other again soon!
Putra was instrumental in early Olivero work. In fact, it was her idea to get it off the ground and *really* do it. I remember the airport discussion with her (post DrupalCon Seattle) when we discussed it. It seems so long ago yet so close.
Here we are pre-FLDC 2020 sprinting. I remember I had a really bad head cold, but I was still stoked to keep working on it
Just pushed styles for "Get started" at mobile widths. This will also need to be a <button> element instead of <nolink>
Removed the commented out stuff from preprocess along with unused code.
Many of the @todo's are gone, but some still remain and will likely be either followup issues, or issues to fix when the underlining issues (could be either browser issues or core issues) are resolved.
Once I have time, I'll get a matrix somewhere so we can comprehensively test, but in the meantime, I'd love as many eyes on this as possible so setting back to NR
That seems like a regression that should be easy to fix before we deploy. It's probably the one visual clue most users will have indicating that they are logged in vs not.
This is now working!
The header is ready for review! This is a comprehensive refactor that does the following:
- One monolith component was split into eight(!!!) components, which is much more manageable
- Greatly increases accessibility
- Changes the top level item element type from span to button, which is semantically correct
- Top level menu items implement aria-expanded. This was previously implemented on the spans, but that attribute is invalid for that role
- Mobile menu disclosure button is now a span
- Allows users with assistive tech to skip over menus if needed (currently submenus open and force users to tab through)
- Proper labels on all buttons
- Proper visible focus states
- Visual changes
- At wide widths, dropdowns are the full width of the header (per design)
- Submenus animate on open/close (note all animations respect
prefers-reduced-motionmedia query) - This works on all browsers (with great effort)
- Only one dropdown is visible at a time
Still to do
- Right now the “Support Drupal” special styling is controlled via checking for the text string in the twig template. This is a bit brittle
- The right hand “Get started” menu is not yet refactored, and still has a lot of accessibility issues.
- Submenus don’t respond to the ESC key. It’s a nice touch if they close when that’s hit.
- The designs for the mobile menu have a background overlay. I have some questions about this before we implement it (fixed? What if people scroll then hit it, etc)
- The user menu icon won’t change to the avatar of the logged in user
- Still needs various accessibility fixes for
forced-colors(Windows high contrast) - All font sizes (and most everything) are in PX. We should have a list of variables that are defined in REMs
Summary
Overall this is a huge improvement from the current version, and is the culmination of a lot of work from both Jayme and I.
What can we do to make those references working outside of Drupal (for example: in storybook)?
Or if we have an image component that we want to be able to use with and without Experience Builder.
Is it possible to have a fallback value? Maybe something like
$ref:
- module://experience_builder#image
- theme://mytheme#imageI'm not going to keep re-rolling this everytime some CSS changes. Let's get a review in before fixing the tests and recompiling the CSS.
Conflicts fixed!
Fixed!
The cause was that Intersection Observer was looking to see if the navbar (which includes the filters) was intersecting the viewport, and if so, stickying it. This cause the navbar to not intersect, which leads to an infinite loop and flickering.
For short viewports, the navbar was intersecting the bottom of the viewport, which I did not take into account.
To fix, I passed in an option to extend the bottom intersection point by 10000px, which is an arbitrary large number that will never be reached.
mherchel → changed the visibility of the branch 3536333-schedules-daysfilter-toolbar to hidden.
Whoa! Good catch. Should be easily resolvable. Will look in a bit. :)
This is ready for review:
A bunch of notes:
- The original author was using GSAP only to manage the stickiness of the schedule filters. This is way overkill.
- I refactored the GSAP out, and replaced it with Intersection Observer, which is a native API. My guess is this wasn't widely available when this JS was written.
- Removing the GSAP saves 100k of JS
- Functionality should be the same, but without the weird bug in the video above.
Will add testing instructions to the IS.
mherchel → created an issue.
mherchel → made their first commit to this issue’s fork.
is in … does that mean this can be closed? 😄
Yeah, this should be closed. i'm assigning credit to everyone (I don't see any unhelpful comments), but I'll leave it to the maintainers to close.
volkswagenchick → credited mherchel → .
This looks great.
Tested with geolocation module.
Feedback:
- Would love to see some simple explainer text stating what Drupal Forge is and how it makes money.
- What does "Express Launch" launch?
- "Free for a limited time" ... does that mean that we can pay to keep it up longer? Is that the business module?
While testing this out, I found that the primary button's focus state is not visible in the hero, because both the focus outline and the background are the same color (Drupal navy).
I fixed that while I'm in here. Should be good to go!
mherchel → created an issue.
This is ready for review in the private Gitlab.
mherchel → created an issue.
I did some extensive spot checking in Olivero, Claro, as well as the off-canvas dialog.
Everything looks as it should. Without some type of robust visual regression system, this is the best we're going to get.
I recommend getting this merged sooner than later, so we have time to identify any potential regressions (which I'd be surprised to see).
Also, this saves about 269kb on the initial page load. It'll save more if the <pre> tag was present to load Source Code Pro
This is ready!
Steps:
- Removed all non-woff2 files
- Cleaned up the global-fonts.css to reflect correct paths
- Swapped out the noto-sans variant of the font to be consistent with the other variants to only contain latin glyphs
- Swapped out the variable TTF for the Source Code Pro font. We weren't using any of the variable font features, and the file was unnecessarily large.
- added
font-display: blockto all the font declarations. This will tell the browser to wait for the webfonts for 3s. If it doesn't have it by then, it will load the fallbacks. If and when the webfonts download, they'll be swapped out.
God that is a fugly selector! Anyway, I'll take a look at this shortly!
I found bugs in Tugboat:
Bugs fixed (🤞)
Good catch!
FYI, test is red because evidently the performance test counts the number of CSS bytes (which is obviously increasing with this MR).
This will need to be changed at https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/navig..., although I don't want to do it just yet, as reviews might necessitate further changes.
New MR created with a "regular" CSS reset.
The "Donut" selector that @nod_ mentioned in Slack isn't necessary.
Because [data-drupal-admin-styles] exists multiple times on each page, we can't use a regular #id selector reset like we do we the off-canvas. Fortunately, there's a "quirk" with the CSS :is() pseudo-class, where its weight is equal to the highest weight selector in the list.
So, if we write :is(#extra-specificity, [data-drupal-admin-styles]), the ruleset will match [data-drupal-admin-styles], but have the specificity of the ID #extra-specificity. This is how we're proceeding.
This should be ready to go!
mherchel → changed the visibility of the branch 3511280-front-end-theme-styles to hidden.
I created a test case, where i test Noto Sans with the non-latin glyphs above.
The tl;dr is that the glyphs will gracefully fallback to system fonts if needed.
IMO, we should only include the latin glyphs, which would save ~1mb of critical assets. The glyphs will be rendered properly, and it's extremely doubtful that anyone will know they're in a different font.
You can see this a short screencast at https://youtu.be/LAPYmm7I8rw
I'm attaching my test files, if anyone wants to test locally.
I asked chatGPT
I have an international website where people from all over the world register their names. Their names might include non-latin glyphs.
I need to test out fonts to make sure the names are rendered properly.
Can you give me a representative list of somewhat common names that use non-latin glyphs?
And it gave me this. I'll use it to test.
Arabic
- محمد
- فاطمة
- علي
- یاسمین
- حسین
Cyrillic
- Алексей
- Наталья
- Дмитрий
- Ольга
- София
Chinese
- 王伟
- 李娜
- 张强
- 陈杰
- 刘洋
Japanese
- 佐藤健
- 田中美咲
- 山口翔
- たかし
- サクラ
Korean
- 김민준
- 이서연
- 박지훈
- 최예은
- 정우진
Devanagari (Hindi)
- आरव
- सिया
- अर्जुन
- प्रिया
- विवेक
Greek
- Γεώργιος
- Αθηνά
- Νίκος
- Ελένη
- Δημήτρης
Hebrew
- יוסי
- שרה
- דוד
- רונית
- משה
Thai
- สมชาย
- พรทิพย์
- สุริยา
- กิตติ
- นพพร
We do need a font with all the glyphs, since people’s names are in many scripts, localize.drupal.org has many languages, and everywhere the global community is communicating.
Yeah, but all of the other font weights (I assume) don't have the glyphs. If we load all or the font files, it could potentially add 1MB of render blocking assets to the page, which IMO is a serious problem.
Is there anyway to get a list of names that have non latin glyphs? Then we can test properly. It's entire possible that the glyphs would gracefully fallback to another font (or the browser will otherwise handle it).
When exporting the font, we have the following options:
- cyrillic
- cyrillic-ext
- devanagari
- greek
- greek-ext
- latin
- latin-ext
- vietnamese
This is ready for review. A couple notes:
- Both mobile and desktop search buttons now toggle
aria-expanded - After opening the search drawer (both mobile and desktop), the search input will now focus.
- Open / close animation is much smoother now. There was some weirdness before that was being solved with a transition delay on the search form.
- I added testing instructions to the IS
Had a long discussion about how to proceed with this at https://drupal.slack.com/archives/C7AB68LJV/p1751487654599299
Involved were navigation maintainers @nod_ and @finnsky (among others).
Per @nod_, we're going to move forward with the reset similar to how the offcanvas does it. We should also look at the fence created by https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/ckedi... and see if this is something we need or could use.
mherchel → created an issue.
mherchel → created an issue.
Good catch!
@biz123 - Let me know if you're still actively working on this. If not, I'm happy to help
I looked over the code, issue, and looked through the changes
A couple notes:
- This is a large MR
- Lots of order changes, which muddies things up
- Most of the work is in css/base/basic-elements.css and changing the global-text class to wysiwyg
- I don't see anything within the code that's going to break anything.
- Looking visually, I don't see any obvious regresssions
My initial thought is that because 1) This MR has been open for two months and there's a lots of reorganization, this should be merged, because it will hold up other changes.
We should also do a followup to convert the old-style CSS to use nesting.
Not yet setting this to RTBC, because not sure if @biz123 thinks its ready (it's not set to review).
nicxvan → credited mherchel → .
the_g_bomb → credited mherchel → .
New changes look good. The table library will be loaded when rich text fields are being displayed.
This looks good to me. I looked at the code, and it looks as expected. I also tested it out and checked out the interfaces with the most "interesting" CSS, to make sure they look as expected, and they do!
CR looks
to me!
Looking at the 11.x, the current top bar is is an <aside> with an aria-labelledby attribute that points to its visually-hidden heading, with the text "Administrative top bar"
From my point of view, I don't see any reason to change this (please correct me if I'm wrong).
There are currently two <nav> elements in the sidebar, which we will consolidate to one.
If all this looks correct, I'll update the IS. I also feel like we might need to have a discussion in Slack to make sure we're on the same page. The requirements of this issue keep changing, and we need to lock it down to make progress.
I hate to bikeshed this.
From @rkoller in #56:
For the sidebar i would use a navigation landmark and label it with
primary, that way it would be crystal clear that this is the main navigation for the admin ui.
I worry that on the front-end of the site, the primary label would be confused with the primary navigation of the site. My thought is it could be named administrative toolbar Thoughts?
mherchel → made their first commit to this issue’s fork.
Yeah, I was also wondering about the 'nesting-rules': { edition: '2021' },. Per the docs it defaults the the behavior of previous PostCSS nesting (which is not actually what browsers implemented).
The new rules (2024-02) cause the browser to accurately reproduce specificity by wrapping parent selectors in an :is(). This mimics the actual behavior of browsers (which basically wrap parent selectors in an :is()).
I really doubt the 2024-02 rules will cause any regressions, but it's impossible to be 100% sure unless we have some type of robust visual regression testing going on, which we don't.
My suggestion is to stick with the 2021 rules for now. Early on after 11.2, lets update to the 2024-02 rules, which will more accurately reproduce the specificity that browser do. Then we'll have several months to find any potential regressions.
After that, we should really see what's holding us back from shipping native nesting. A quick look at https://caniuse.com/css-nesting, shows that our supported browsers support it.
OK. Should be good!
Fixing now
I like the naming in #35!
Should there be a reverse relationship too? An example of this would be to limit people to put accordion_items only into accordion_group components.
Good catch!
The issue was that the top bar was rendering, even if it didn't have content, which pushed down the popover.
I added a check for content within the twig file. Should be good to go. Thank you!
Yay! Test is passing. Note that I don't test out displace's functionality (that gets tested in its respective tests), I just test that the attribute is added properly, which is what the JS in this module does.
Yeah the current tests passed when I re-ran them.
Just pushed a simple nightwatch test. Lets see if this passes (I didn't test it locally 😬🤞)
Ready for review! Screenshot attached :D
mherchel → changed the visibility of the branch 3526266-navigation-top-bar to hidden.
mherchel → created an issue.
Fix attached.
mherchel → changed the visibility of the branch 3526180-regression-drupal.displace-not to active.
mherchel → changed the visibility of the branch 3526180-regression-drupal.displace-not to hidden.
mherchel → created an issue.
Somehow I missed this.
Tables can also be added via CKEditor, which is why we had it as a global library. You can see at https://git.drupalcode.org/project/drupal/-/blob/11.2.x/core/themes/oliv..., we're also targeting tables that appear in .text-content (which is formatted text).
wim leers → credited mherchel → .
mherchel → created an issue.
mherchel → created an issue.
mherchel → created an issue.
wim leers → credited mherchel → .
andyg5000 → credited mherchel → .
Heck yes! This totally fixes the issue for me. I'm attaching two videos:
- Part 1: The video showing the issue existing
- Part 2: The video showing patch installed, and the issue solved
Still leaving a NR, since we need a code review and I'm not super qualified for this level of PHP. But confirmed to fix the issue!
evidently the test case isn't as simple as I initially thought. I've run into this a lot, but need to see what the differentiator is that causes it.
Updating the steps to reproduce
Slack discussion at https://drupal.slack.com/archives/C079NQPQUEN/p1746998199858839
In it, backend framework manager @larowlan states (in regards to @deviantintegral approach in the issue summary)
that approach looks fine to me, ideally with the cache clearer injected
Bumping this to Major (note it may need to be critical). Most new themes will have SDCs, and this will affect anyone who install new themes via the UI.
smustgrave → credited mherchel → .
Any chance this can be backported to 10.x? It'd be nice to use this features and have themes be compatible with both D10 and D11. My understanding is that XB hopes to support D10 also.
This works perfectly!
I ran the command in ddev, and worked as expected.
The theme appeared under the appearance route as expected.
I played around with it and tried to break it, and it worked great!
Running this thing through its paces 🤞
➕💯➕💯➕💯➕💯
mherchel → created an issue. See original summary → .