- @lauriii opened merge request.
- 🇺🇸United States mherchel Gainesville, FL, US
This is looking really good. A couple things that need to be fixed.
- Olivero styles need a lot of work. The extra-small variant isn't small, there's no focus state on the toggle button, etc
- The toggle icon doesn't isn't apparent in high contrast mode,.
- The component is pretty much unusable within Stark. We should add some default styles so that the component is usable.
- There's a lot of directional styles that should be using CSS Logical properties.
- What is the non-js behavior supposed to be? Right now, the styles are kinda weird. Not sure they're correct.
- We also need to add styles specific to the off-canvas dialog, as all of those styles will get reset.
- 🇺🇸United States mherchel Gainesville, FL, US
Talked with @lauriii and we came to the conclusion that we need to do the following to get this over the finish line.
- Remove hover variant - move to followup. This is currently unnecessary, and has issues on touch devices, and focus issues
- Refactor JS to remove support for IE
- Remove wrapping div around main link and trigger button (if possible)
- Add basic styles for stark
- Convert to CSS logical properties
- Styles for off-canvas
- Fix olivero styles
- Non-JS styles should look like the regular split button in an open state
- First commit to issue fork.
- 🇺🇸United States hooroomoo
Created https://www.drupal.org/project/drupal/issues/3347690 ✨ [PP-1] Create hover variants for splitbuttons Postponed as a follow up
Refactor JS to remove support for IE
Remove wrapping div around main link and trigger button (if possible)
Styles for off-canvas
Non-JS styles should look like the regular split button in an open state - last update
over 1 year ago 29,380 pass, 2 fail - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago 29,384 pass, 2 fail - 🇺🇸United States bnjmnm Ann Arbor, MI
No-JS Styles
Claro
Olivero
Stark
Off-Canvas Styles
Claro
Olivero
- last update
over 1 year ago 29,384 pass, 2 fail - last update
over 1 year ago CI aborted - last update
over 1 year ago CI aborted - last update
over 1 year ago 29,385 pass, 1 fail - Status changed to Needs review
over 1 year ago 5:17pm 2 May 2023 - last update
over 1 year ago 29,381 pass - last update
over 1 year ago 29,379 pass - 🇺🇸United States hooroomoo
It looks pretty good, I just noticed a few minor things when manually testing.
1. Visual focus is lost when tabbing through the splitbutton dropdown options when JavaScript is disabled
2. Off canvas styling (text covering the toggle in small button and different colors on the button in olivero)
3. Stark element extending splitbutton looks a little off
- Status changed to Needs work
over 1 year ago 6:07pm 4 May 2023 - Status changed to Needs review
over 1 year ago 6:08pm 4 May 2023 - last update
over 1 year ago 29,386 pass - Status changed to Needs work
over 1 year ago 7:18pm 5 May 2023 - 🇺🇸United States mherchel Gainesville, FL, US
In the commits above:
- Refactored the base styles to use logical properties, have some better initial styling, and be usable in forced colors.
- Refactored Claro's styles to take into account new base styles. Also made usable in forced colors.
- Removed the exception for the splitbutton from the off-canvas reset.
Still to do
- Refactor Olivero's styles
- Off-canvas styling for splitbutton (that doesn't inherit the theme's styling).
- last update
over 1 year ago 29,386 pass - 🇺🇸United States mherchel Gainesville, FL, US
Got some scaffolding in place for the initial styling of the off-canvas splitbutton. Still lots of work to do there, but it's coming along.
- last update
over 1 year ago 29,386 pass - last update
over 1 year ago 29,387 pass - last update
over 1 year ago 29,387 pass - last update
over 1 year ago 29,387 pass - 🇺🇸United States mherchel Gainesville, FL, US
Pushed some work on making the splitbutton look good in both Olivero and in the off-canvas menu.
It's looking pretty good now (including RTL and focus styles), however I still want to put in a bit more TLC into abstracting the variables, and simplifying the CSS, so leaving at NW till next week.
- last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago 29,388 pass - last update
over 1 year ago 29,388 pass - last update
over 1 year ago 29,389 pass - 🇺🇸United States mherchel Gainesville, FL, US
Refactored the base styles to use PostCSS and have properties defined by custom CSS properties
- 🇺🇸United States cosmicdreams Minneapolis/St. Paul
Hi all, just found this issue so I'm sorry if my question was asked before.
What if we wanted to have the options in a splitbutton store a user preference? Would it be possible to:
- Include a checkbox within the button of a splitbutton?
- Write javascript to handle the clicking of that button so it can store the current state of the user preference in localStorage?
- Be allowed to provide different background colors to provide a visual cue for what state the setting is currently in?
- 🇺🇸United States mherchel Gainesville, FL, US
Cross linking 🐛 Dropbutton quickly shows/hides its menu on pageload causing layout shift Fixed . The dropbutton component has an issue where the non-JS styles cause a layout shift. Split button will have the same issue, and we should probably take care of it before adding to core.
- 🇺🇸United States bnjmnm Ann Arbor, MI
Re @cosmicdreams from #174, while nothing you've listed is supported out of the box, the splitbutton architecture is intentionally designed to accommodate this kind of flexibility. It's great to know about use cases such as yours that make this a benefit to core.
- Include a checkbox within the buttons of a splitbutton? (Assuming a split button is a collection of button children) links, buttons and
input[type=submit]
are the the elements that are tested and have default styling, but fortunately it's not exclusive to those elements. - Write javascript to handle the clicking of that button so it can store the current state of the user preference in localStorage? That sounds like it should just require a pretty standard application of JavaScript. You'd want to add the appropriate listener for the element that directly impacts the user setting. It being contained in a splitbutton will not disrupt the splitbutton unless there are scenarios where it MUST be visible even if the button isn't open... but there are ways around that if it comes up (find me on Drupal Slack 🙂)
- Be allowed to provide different background colors to provide a visual cue for what state the setting is currently in?
This could potentially be a simple as running some js in a behavior to add a class as needed to the element, and since it's in a collapsed element you don't have to worry about it flashing as a different color. If it needs to be done earlier, check out toolbar.module for some wild-but-effective ways of letting localstorage change styling before the first page render (which I can explain further if needed.. again find me on Drupal Slack 🙂
- Include a checkbox within the buttons of a splitbutton? (Assuming a split button is a collection of button children) links, buttons and
- 🇺🇸United States cosmicdreams Minneapolis/St. Paul
@bnjmnm it is great to hear that the intent of the splitbutton is to be flexible and allow more elements that the currently supported: link, button (which I learned recently isn't a but is basically a submit button) and literally a submit button. I don't see how that flexibility is achieved without overriding the splitbutton element's filterItems method though.
see: https://git.drupalcode.org/project/drupal/-/merge_requests/3377/diffs#c5...
That method is what provides the limitation of the render element to support ONLY those three #types. If you wanted to maximize your flexibility you might want to consider adding
html_tag
to the list. Then you could have all kinds of things. Or make the$allowed_types
a property on the object so that overriding objects would just need to override that property and keep everything else the same.In working on same_page_preview, I've had to turn to javascript to provide me the kind of flexibility that I need.
- 🇺🇸United States cosmicdreams Minneapolis/St. Paul
Also; Hello! is this native nested CSS in core? Yes please.
Is there an initiative to spread this enhancement to all of Drupal's CSS yet? That makes me think back to my first contributions in Drupal where we used SCSS to "modernize" Drupal's CSS.
- Open on Drupal.org →Environment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Waiting for branch to pass - last update
over 1 year ago 29,403 pass - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - Open on Drupal.org →Environment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Not currently mergeable. - Open on Drupal.org →Environment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Not currently mergeable. - Open on Drupal.org →Environment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Not currently mergeable. - last update
over 1 year ago 29,431 pass, 3 fail - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - 🇺🇸United States bnjmnm Ann Arbor, MI
Claro and Umami are currently looking a little funky, which I suspect is due to some of the in-progress changes recently added by @mherchel. Perhaps that can either get wrapped up or provide a comment describing what needs to be done to complete the intended changes?
- last update
over 1 year ago 29,439 pass - 🇺🇸United States mherchel Gainesville, FL, US
exactly. I've been going back and forth in between Olivero and Stark and tweaking the default styles' custom properties to make it easily themable by all themes.
Still haven't gotten back yet to Umami or Claro.
- last update
over 1 year ago Custom Commands Failed - 🇺🇸United States mherchel Gainesville, FL, US
Just pushed my local changes to the base styles and Olivero's styles. I also pushed a JS fix to not close the dropdown if the dropdown contains focus (we had to do something similar within Olivero's menus).
Still to do:
- Re-do dropdown styles to take into account base style changes
- Same with Claro
- Same with Umami
I feel that we're starting to get really close here!
- last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago 29,445 pass - Status changed to Needs review
over 1 year ago 2:01pm 25 June 2023 - 🇺🇸United States mherchel Gainesville, FL, US
CSS is much cleaned up and default styles are looking good. Setting back to NR
- last update
over 1 year ago Custom Commands Failed - Status changed to Needs work
over 1 year ago 2:25pm 29 June 2023 - last update
over 1 year ago Custom Commands Failed - Open on Drupal.org →Environment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Not currently mergeable. - last update
over 1 year ago Composer error. Unable to continue. - Open on Drupal.org →Environment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Not currently mergeable. - Merge request !4297Issue #1899236: Add new Splitbutton render element to eventually replace Dropbutton → (Open) created by bnjmnm
- last update
over 1 year ago 29,569 pass, 1 fail - last update
over 1 year ago CI error - last update
over 1 year ago 499 pass, 1 fail - last update
over 1 year ago 499 pass, 1 fail - last update
over 1 year ago 499 pass, 1 fail - last update
over 1 year ago 505 pass - 🇺🇸United States bnjmnm Ann Arbor, MI
To prepare for the dependency evaluation, I posted an issue to the Popover polyfill project to see what their security policies are https://github.com/oddbird/popover-polyfill/issues/110
- last update
over 1 year ago 505 pass - last update
over 1 year ago 29,811 pass - Status changed to Needs review
over 1 year ago 1:59pm 7 July 2023 - 🇺🇸United States bnjmnm Ann Arbor, MI
CSS got overhauled.
Visibility & stacking is now managed by the popover API (polyfill dependency evaluation in IS). The version of Chromedriver on DrupalCI does not support popovers, so this effectively tests the polyfill, too.
Positioning is now managed by floating UI
This significantly reduces the unwanted surprises of a splitbutton conflicting with another z-index, causing unwanted reflows, or being inaccessible to due position on a page.
This is again reviewable.
Stark
Claro
Olivero
Umami
- Status changed to Needs work
over 1 year ago 4:53pm 17 July 2023 - 🇺🇸United States smustgrave
Wonder if those screenshots could be added to the CR?
- Status changed to Needs review
over 1 year ago 7:09pm 17 July 2023 - 🇺🇸United States mherchel Gainesville, FL, US
Wonder if those screenshots could be added to the CR?
This is a great idea, but the MR still needs review.
- Status changed to Needs work
over 1 year ago 11:51am 8 August 2023 The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- First commit to issue fork.
7:16 6:08 Running- last update
over 1 year ago 25,945 pass, 1,352 fail - Status changed to Needs review
over 1 year ago 5:55am 11 September 2023 - Status changed to Needs work
over 1 year ago 2:59pm 11 September 2023 - First commit to issue fork.
- 🇬🇧United Kingdom catch
Is the polyfill still needed here, it looks like popover has support in all supported browsers now?
- 🇷🇸Serbia finnsky
Cool! I just removed polyfill from toggletip
https://www.drupal.org/project/drupal/issues/3197758 ✨ Create a new component: Toggletip RTBC - 🇺🇸United States mherchel Gainesville, FL, US
Is the polyfill still needed here, it looks like popover has support in all supported browsers now?
As of now, it's needed. But I'd guess that it won't be needed by the time this finally lands.
- Only supported by Safari 17.5. Right now, Drupal supports the last two major versions of Safari. But... Safari 18 is going to come out this fall
- Only supported by Firefox 128. However, Drupal supports the latest release of Firefox ESR, which is currently at 115. But, the next ESR release is due this September, and will support it.
So... TLDR, we need it now, but likely won't need it in a few months as newer versions of the browsers get released.
- First commit to issue fork.