I updated the title, and added reasoning to the CR. Thanks!
heck yeah. There were lots that were missed, I pushed a fix for that.
Will ping @mgifford in Slack to see if he can signoff on the implementation.
I spun my wheels on this for sooo long. I couldn't figure out how to set it to default. I was looking in the configuration form.
Anyway, for anyone who finds this via Google, the option is under the dropbutton.
Pushed a new commit adding a left border for the active menu item.
@kentr Do you want to open up a new MR?
If you're not familiar with either the MR process or contributing code to core, I'm more than happy to help!
Pushed another commit to ensure that the toolbar-button component uses the proper system color within forced colors (e.g. linkText for links and buttonText for buttons)
Pushed another commit ensuring that the title icon is visible in high contrast. Hopefully good to go!
Tooltip border added. Hopefully good to go!
The three dots in the top bar's contextual menu is responding to the forced colors. It's set to buttonText which is a system color in CSS that responds to forced colors. I think it's just small (as you said), so it's not highly visible.
Pushing a fix for the tooltip in a sec!
Added a walk though of testing at https://www.youtube.com/watch?v=v4dTY4__F6U
Merry Christmas and Happy Holidays!
Changes for forced-colors are made. This mainly involves adding transparent borders (which are made visible in forced-colors), and adding proper fills for SVGs.
mherchel → changed the visibility of the branch 3438477-high-contrast-mode to hidden.
FWIW, I don't believe we should use the group key as tags for expected.
The group key organizes the components within the Ui. Which is a very different task than an expected tag.
An example of this is I want the tab_group and tab_item components to be in the same folder within the builder's UI, however, tab_item might have a tag that's in the tab_group's expected value
CSS variables won't get reset by this. But regular properties would.
You could also wrap it in something like
:is(#specificity-hack, *) {
.my-regular-selector {
...
}
}
But you want to make sure it doesn't bleed into the front-end theme.
Fixed with CSS.
The proper fix would probably to go to whatever's injecting the data-offset-top, but there's no way to know if it's just Layout Builder, or if there are also contrib modules out there that might have copied LB's implementation.
So, fixed with CSS.
Adding image. I can fix this in CSS, but no clue why it has data-offset-top="0"
mherchel → created an issue.
Change record (currently draft) at https://www.drupal.org/node/3560492 →
The core issue is that we added a CSS reset with the specificity of an ID to prevent front-end styles from bleeding in.
We'll need to explicitly add CSS specificity into Gin assuming Gin wants to keep on affecting styles within Navigation's top bar, or sidebar.
Yeah, Gin will need to add specificity. I wonder if we should have a CR
Setting back to NW. Just discovered that at wide widths, and the navigation is in its "narrow" mode, it's set to inert, which makes it inoperable.
will work here because we have a collapse animation. It's gone now. Perhaps we could add visibility:hidden with some delay.
visibility supports CSS transitions. So it can work with visibility (we do so within Olivero).
But, the solution within the MR (toggling inert via JS) works just as well.
Tested this out, and works well!
I'm all for this. But we need to make it as accessible as possible. There's a ton of gotchas with infinite scroll
You can also test this on Chromium dev tools FYI: https://devtoolstips.org/tips/en/emulate-forced-colors/
Added check for when offcanvas is at top of screen.
OK. Easy fix. Hopefully we can get this in for 11.3.
mherchel → changed the visibility of the branch 11.x to hidden.
mherchel → changed the visibility of the branch 3531516-proper-fix-seriously to hidden.
mherchel → changed the visibility of the branch 3531516-proper-fix-seriously to hidden.
mherchel → changed the visibility of the branch 3531516-fix-displace-logic to hidden.
From @catch in #47
I'm not sure what tests would look like for this (that would actually prevent the regression) except for visual regression tests that we don't have a framework for. If someone does, it would be great to add them here. If there's not a good way to add tests, we should open an explicit follow-up to add them later and add it to the navigation stable meta somewhere.
Agree. No easy way to currently add tests, and TBH, I don't even believe this is something that will likely break. I'll add a followup
I'm going to fix this.
A bit of background to address the comments above:
What is Drupal.displace() and how does it work.
Drupal.dispace() is a JS function that looks for attributes like data-offset-top, data-offset-left, etc. These are meant to be used by "sticky" position: fixed admin UI elements. And if used properly, will inform the front-end code how to properly place itself on the top of the page. For example, we don't want a fixed header to obscure the admin toolbar (or vice-versa).
So when Drupal.displace() is ran, it'll look for those data-offset properties, return the object, and also update those CSS vars that you're seeing.
(I should really turn this into a blog post).
Anyway, since the navigation's toolbar is fixed to the top, we definitely want that attribute on it.
Why was that line of CSS introduced?
To be honest, I might have done that back in the day (it looks like something that I would have written). When this was written, the top bar did not yet exist. And even though that didn't exist, there are other top bars (e.g. the old workspaces UI). So we needed to accommodate that.
For the Navigation module, the left toolbar is expected to go full height -- even if the the top bar exists. So the fix is simple. We need to just remove the one line of CSS.
Yay! Lets get this in before it gets stale!
This is ready for review! I did extensive testing, and it should be good to go!
Lets get this reviewed, and committed asap. It touches every single CSS, so it's easy to get out of date (and I've already re-done it 3x now!)
mherchel → changed the visibility of the branch 3511280-nav-specificity to hidden.
Current MR is all FUBAR. Going to create a new one.
mherchel → changed the visibility of the branch 3511280-regular-css-reset to hidden.
Sounds good. I'll likely be able to work on it this weekend. Thanks :)
mherchel → created an issue. See original summary → .
mherchel → created an issue.
Removed directive that instructs the AI to always fill every available slot.
mherchel → made their first commit to this issue’s fork.
This works great for the most part. Thank you!
The only issue that I am running into is when I'm in the middle of panning, if the mouse goes over either Canvas sidebar (e.g. the components list, or the props), the panning stops. I compared this against Figma, and within that, the panning continues as expected. Figma also gives a nice little "Drag" cursor that indicates the state to the user.
Is it possible to fix either of these?
Youtube video of this (from NEDCamp) is at https://www.youtube.com/watch?v=T0YhqzdxYvQ, in case I didn't explain myself clearly.
Tested in FF and Chrome! Works!
@q0rban is now qualified for 📌 Buy Canvas developers drinks at Drupal events Active !
Doesn't appear to be a way to detect support for @scope using CSS like @support.
Chrome has UA padding, so we just just code that in the input's CSS.
Root issue is that we're using @scope somewhere, which the latest Firefox does not support.
@q0rban and I are working on this at NEDCamp
Tested under Chrome, Safari ,and Firefox at NEDCamp contribution sprint.
It works! Thanks for fixing this papercut!
mherchel → created an issue.
+1 on this
Another Slack thread with the same issue: https://drupal.slack.com/archives/C072JMEPUS1/p1762551589103589
These are seasoned Drupal devs. There's definitely a UX issue here.
A couple notes on your messages:
Warning: The xxx role is unnecessary for element xxx
These are leftover role attributes from earlier versions of Safari, which didn't properly set up the aria roles. Note this won't cause any issues as the attributes match the semantic meaning of the element. But at some point they could be removed.
Error: Element button is missing one or more of the following attributes: type.
I have no clue what this is? Did you run your report while logged in? It might be from a contrib module.
Error: Attribute fetchpriority not allowed on element img at this point.
'
Why not?
No worries. moving the issue.
mherchel → created an issue.
Hey @phil stringer
This project was for before Olivero was in Drupal core. Are you using the Drupal core version of the theme, or did you pull down this contrib theme?
it can be left up to the caller to just not pass NULL values.
It can... but it's not always easy.
We frequently call components from templates where props may or may not be populated. If they're not populated, a null is passed, which makes us have to add"null" as a type (which is bad DX).
I feel that the the DX wins here far outweigh any negatives (which TBH, i don't see)
I think this is a great idea... I'm wondering how we identify these people at said Drupal events? Do they have a decoder ring or does a list exist we can point people to?
This is true. Maybe they have a secret handshake? If so, i'm not sure how we'd know if it's correct.
To be on the safe side, we should buy a person drinks just even if we suspect they're on the team.
Per @penyaskito in https://drupal.slack.com/archives/C072JMEPUS1/p1762261704549799?thread_t...
this is yet another issue where https://www.drupal.org/project/drupal/issues/3443882 ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active would be helpful
Just pushed some changes. This should be good to go!
Thank you for the heavy lifting on this @quietone!
Props work perfectly as far as I can tell.
Is this something that's reasonable to change? Doing includes is a normal way to split up logic, and it's a decent workaround to the $ref issue.
Adding Youtube video explaining the bug: https://www.youtube.com/watch?v=0Yop2IxjegE
Adding related issue ✨ [PP-1] Allow schema references in Single Directory Component prop schemas Postponed
My use case that I need to figure out:
I have a single block (e.g. main menu) that I need to display this with a different component in different places.
Currently, we check the region using the method above, and load a different template (which load different SDCs).
Is there another way to accomplish this with Canvas?
Can confirm this fixes the issue for me! I had to manually apply the patch and then run ddev xb-ui-build. But works as expected! 🙌
Leaving at NR, since I don't have enough context to do a code review.
Thank you!
We can do it differently, we can have a SDC container which has one slot for all columns.
and SDC child column. And no need to ask the editor how many columns he needs.
Good question. The component that we have also asks how they arrange the slots. For example, we might have a 3 column layout that arranges them 25% 25% 50%, etc.
Plus we might want to place more than one other component within a slot.
Couple reasons:
- Developer experience. Sometimes I want to inspect the page to make sure the component is rendering properly under the hood. The preview has pointer-events: none, so its tougher to inspect (although maybe I should file an issue for that).
- Just because I want to see the actual page that my customers will see.
This is something that most SDC authors would make use of, and it would result in better UX's for end users.