Add color theme support to Search & Filter component
Done
Adds color theme support to Search & Filter component by using new color theme variables
- Color of the search and filter panel headers (it is slightly more contrasted from background now, using default text color rather than
#666) - Color of the overflow count - it now uses
$colors--theme--border-informationrather than$color-informationwhich is unthemed and does not appear nicely on the dark theme - Background color of opened search and filter panel (now supports dark theme using themed background color instead of hardcoded to light theme)
- Background color of the search prompt (now uses themed alt background color instead of hardcoded to
#f7f7f7) - Background color of the search clear button (now uses themed input backgroundcolor instead of hardcoded whitesmoke/
f5f5f5) - Color of search and filter panel section bottom border (now uses low contrast border instead of
mid-x-light) - Enabled color theme switcher on search & filter examples
- As a drive-by, added the global variable
$border-high-contrastand replaced all usages of$input-border-thickness solid $colors--theme--border-high-contrastwith$border-high-contrast.
Fixes WD-11868
QA
- Open all search and filter examples and verify that the changes above appear as expected in all color themes.
Check if PR is ready for release
If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:
- [x] PR should have one of the following labels to automatically categorise it in release notes:
Feature 🎁,Breaking Change 💣,Bug 🐛,Documentation 📝,Maintenance 🔨.
- [x] Vanilla version in
package.jsonshould be updated relative to the most recent release, following semver convention:- if CSS class names are not changed it can be bugfix relesase (x.x.X)
- if CSS class names are changed/added/removed it should be minor version (x.X.0)
- see the wiki for more details
- [ ] Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.
Screenshots
I recognize most of the above aren't related to theming directly, but just wanted to capture them during this review.
@pastelcyborg This is a weird component in Vanilla sense. It was originally built in React and its styling has been upstreamed to Vanilla, but there is way too much functionality to it to replicate it in full in plain JS here.
So all we did is create some static examples of different states the component can be to make sure all the styling is in place, but none of the examples is fully funcional (outside of React). So there certainly can be issues if you try to interact with the component.
If you are interested you could try similar tests in React examples and if there are any bugs report them in React components directly. At this point in Vanilla CSS we are only focusing on styling of it.
I don't know if there is a easy better way to approach such component. I guess ideally we should be able to just embed the React component, as this component has practically no use as pure HTML/CSS.
Good to know, thanks @bartaz. Optimally I think the first 2 of my bullets should still be fixed here, as they're related to visual presentation/theming and not any JS functionality.
- I notice there's a "+2" in-field helper on Dark and Paper themes that's not visible on Light theme. Is this possibly missing styles?
@pastelcyborg This also happens in production and seems to be fixed if you reduce page width slightly. It looks like a z-index issue where the "Search and filter" placeholder text is hiding the overflow hint behind it. You can fix it by manually setting the z-index of the p-search-and-filter__selected-count higher in dev tools.
EDIT: On second look, it appears to be intended behavior that this +2 is hidden, as it looks quite broken when overlaid on top of the "Search and filter" placeholder. I think I need to investigate the themes to make sure that it is hidden on dark and paper in this case. Or maybe it is an artifact of the example search and filter JS.
https://github.com/canonical/vanilla-framework/assets/46915153/8ceef3bb-d70c-4462-8ac5-ace701ef5a9f
CC @bartaz
The issue with seeing the +2 on dark and paper is because light theme background for the search input p-search-and-filter__input is completely opaque, while dark & paper have slight transparency. This causes the +2 to be visible behind the search input.
The issue comes from these variables having transparency added:
https://github.com/canonical/vanilla-framework/blob/0803feec5ae5c488c15d9aeb0bbdb0ee538f1c2c/scss/_settings_colors.scss#L177
Results in rgba(255, 255, 255, 0.04)
https://github.com/canonical/vanilla-framework/blob/0803feec5ae5c488c15d9aeb0bbdb0ee538f1c2c/scss/_settings_colors.scss#L225
Results in rgba(0, 0, 0, 0.07)
Whereas light theme inputs background does not have transparency:
https://github.com/canonical/vanilla-framework/blob/0803feec5ae5c488c15d9aeb0bbdb0ee538f1c2c/scss/_settings_colors.scss#L118
Results in rgb(238, 238, 238)
I've attempted to fix the chip overflow indicator problem by introducing an opaque version of the inputs background for each color theme and setting the background of p-search-and-filter__input to it. This appears to solve the issue, but please review and let me know what you think @bartaz @lyubomir-popov @pastelcyborg
As for @pastelcyborg 's second bullet point:
Some crazy stuff occurs if you Tab while within the search field - a "Search" button is focused, but it's about half cut off on the right side of the input, and the "Search and filter" placeholder becomes unevenly vertically aligned
Maybe we should make a new issue for this and solve it separately, since it isn't related to the theming.
What was the +2 showing up behind the input? I woudn't expect anything to be behind it if it's covered - maybe that's something we can fix in the component itself rather than by introducing new colours?
But it is another situation when we have issues caused by transparent backgrounds. I think we need a clear direction on this. And I believe that if we address it, it should be the other way around - backgrounds should be opaque by default, and only if transparent version is needed for any reason there would be a specific transparent version and it would be clear that it is transparent (in all themes).
@bartaz
What was the
+2showing up behind the input?
It was the p-search-and-filter__selected-count from the chip overflow example. In the example, it is always in the DOM and set to visible, but covered by p-search-and-filter__input when it is visible. However this only works on light theme, as dark & paper input background colors have transparency that makes the chip overflow count visible.
backgrounds should be opaque by default, and only if transparent version is needed for any reason there would be a specific transparent version and it would be clear that it is transparent (in all themes).
I agree; I suppose we may need to adjust the values used for the inputs backgrounds on dark & paper. I wonder if there are any apps/sites that are relying on that transparency... I hope not.
@lyubomir-popov When you have some time, please look through the conversation in this PR and let me know your thoughts on best direction forward for this
Color of the search and filter panel headers (it is slightly more contrasted from background now, using default text color rather than #666) - I think these should use the muted text colour Color of the overflow count - just the normal link colour should work fine Background color of opened search and filter panel (now supports dark theme using themed background color instead of hardcoded to light theme) - can we use the input background? Background color of the search prompt can we use the input background? Background color of the search clear button (now uses themed input backgroundcolor instead of hardcoded whitesmoke/f5f5f5) themed input sounds ccorrect, same as the two above. Color of search and filter panel section bottom border - any borders on non-interactive elements should either use the default or low-contrast border styling
As a drive-by, added the global variable $border-high-contrast and replaced all usages of $input-border-thickness solid $colors--theme--border-high-contrast with $border-high-contrast. don't we already have that at the theme level?
Need to adjust so the overflow count is on the baseline
Updated to use flattened inputs background color on paper & dark per design suggestion. This partially fixes the transparency problem, but there is still an issue on hover state due to the dark & paper themes using a partially transparent hover color:
That will need to be updated to a flattened color as well.
@pastelcyborg and @bartaz I've updated this per suggestions from Lyubo - have a look if you like, but we should wait to merge this until @lyubomir-popov is back from his leave and can review it
For now just review the colors updates; I still need to fix the baseline grid alignment of the chip overflow as a drive-by that Lyubo requested
Baseline grid alignment adjusted!
@lyubomir-popov Ready for another design review, the one thing I'm not sure about is the opened search panel background color - it being the same color as the search prompt is a bit strange
I think the color vals I used here are slightly off, as there's a lot of change in unrelated dark/paper examples that use inputs. I'll adjust to avoid that change.
Needed to add new overrides to the JAAS application layout example's colors, as the current example is relying on background color transparency to achieve its look.
managed to break this one - pressing tab to move the focus away leads to the text being misaligned:
also, the focus moves to a previously invisible element, making it visible
driveby: can we make chip borders 1.5px (using $input-border-thickness)? They differ from all other interactive elements that have borders:
managed to break this one - pressing tab to move the focus away leads to the text being misaligned:
also, the focus moves to a previously invisible element, making it visible
@lyubomir-popov I think this is related to 5289, not the theming
driveby: can we make chip borders 1.5px (using $input-border-thickness)? They differ from all other interactive elements that have borders
@lyubomir-popov Sure thing, this has been added to the PR
Apart from the glitches that we'll tackle separately, looks good!
EDIT: just noticed these use h3 - can we please switch to h5 (not necessarily in the markup, could be with p-heading--5 if there's a strong reason why these should be h3s semantically)
just noticed these use h3 - can we please switch to h5 (not necessarily in the markup, could be with p-heading--5 if there's a strong reason why these should be h3s semantically)
@lyubomir-popov Updated to use h5 text. This doesn't change the font size (it was previously manually set to 1rem anyway), but it does slightly alter the line height and other spacing. I set this in the scss (the heading class now extends the h5 behavior) and also updated the markup for clarity.
@pastelcyborg does the commit I added per @lyubomir-popov feedback after your code +1 look good?
Also, this one will need a pretty hefty Percy review, lots of things changing around slightly due to input background colors being tweaked.
@pastelcyborg does the commit I added per @lyubomir-popov feedback after your code +1 look good?
Also, this one will need a pretty hefty Percy review, lots of things changing around slightly due to input background colors being tweaked.
Actually that's a fair thing to double-check - we should wait for confirmation from @lyubomir-popov, since it does change the spacing a bit.
Well spotted @pastelcyborg ! The margin is reset to 0, throwing things off the baseline grid. I don't think that's necessary - here it is with the default h5 margin-bottom:
To me that looks better.
@lyubomir-popov I've updated to remove the collapsed bottom margin from the headings, in favor of using the default h5 margin bottom of 1.125rem
@bartaz I've updated this to remove the custom color from the JAAS aside navigation in the application layout example, as requested.