-
Notifications
You must be signed in to change notification settings - Fork 167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add color theme support to Search & Filter component #5127
Conversation
ebd65cf
to
48563d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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?
- 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
- In addition to the previous bullet, hitting Tab while focused within the default component shifts focus to the Search button, also hiding the chips pane; as it is, I don't believe a user would be able to use this component in an accessible manner due to this
- It's possible to get the search input/chips into a state where input doesn't turn into chips and will - seemingly randomly - remove field input; this is most obvious on the With Search Prompt example if you alternate between clicking the chip, typing into the input, clicking the Search for... link, and refreshing the page
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. |
@pastelcyborg This also happens in production and seems to be fixed if you reduce page width slightly. It looks like a 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. Screen.Recording.2024-06-13.at.11.44.30.AM.movCC @bartaz |
The issue with seeing the +2 on dark and paper is because light theme background for the search input The issue comes from these variables having transparency added: vanilla-framework/scss/_settings_colors.scss Line 177 in 0803fee
Results in rgba(255, 255, 255, 0.04)
vanilla-framework/scss/_settings_colors.scss Line 225 in 0803fee
Results in rgba(0, 0, 0, 0.07)
Whereas light theme inputs background does not have transparency: vanilla-framework/scss/_settings_colors.scss Line 118 in 0803fee
Results in rgb(238, 238, 238)
|
48563d0
to
d5fea43
Compare
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 As for @pastelcyborg 's second bullet point:
Maybe we should make a new issue for this and solve it separately, since it isn't related to the theming. |
What was the 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). |
It was the
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 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 |
d5fea43
to
73cd1e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inline-commenting where to switch to input background color for my own sake
…parency search input" This reverts commit 8bc563b02a9f7981a98414d5f041013b40c3399d.
…er-high-contrast" This reverts commit 1481cc6087ec31c4f06349b3802e49400457637f.
This reverts commit e672a21.
f0e4bd8
to
050fbfd
Compare
@bartaz I've updated this to remove the custom color from the JAAS aside navigation in the application layout example, as requested. |
Done
Adds color theme support to Search & Filter component by using new color theme variables
#666
)$colors--theme--link-default
rather than$color-information
which is unthemed and does not appear nicely on the dark theme#f7f7f7
)f5f5f5
)mid-x-light
)Fixes WD-11868
QA
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:
Feature 🎁
,Breaking Change 💣
,Bug 🐛
,Documentation 📝
,Maintenance 🔨
.package.json
should be updated relative to the most recent release, following semver convention:Screenshots