-
Notifications
You must be signed in to change notification settings - Fork 166
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
Paper background #4763
Paper background #4763
Conversation
Demo starting at https://vanilla-framework-4763.demos.haus |
Recreated from #4747 |
* Update rule-highlight pattern to use to appropriate class * Fix missing highlight rule in the example --------- Co-authored-by: Bartek Szopka <bartek.szopka@canonical.com>
Edited to point at |
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.
Looking good!
@@ -7,7 +7,7 @@ | |||
|
|||
<div class="col-8"> | |||
<div class="p-form__control"> | |||
<input type="text" id="full-name-stacked" name="fullName" autocomplete="name"> | |||
<input {% if is_paper %}class="on-paper"{% endif %} type="text" id="full-name-stacked" name="fullName" autocomplete="name"> |
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.
It's a bit fragile to have to add a on-paper
class to every element
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.
It's the same with any other theme we have, it's the element that needs to decide on its style.
We can't rely on the fact that parent body has is-paper
class name, because there may be a theme change in between. Input may be on dark strip, or on white background, or in a card.
The only reliable way to determine element style is to define it on the element level.
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.
Personally, I think we can determine the situations in which the elements will be used and can rely on the parent styling. Similar to the p-strip--dark
pattern. If the pattern sets the background and it should set the color. If an input is within a card and the card sets the background the card should deal with that situation.
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.
The issue is that it goes beyond just text colour, this affects whole components.
So now every time something changes background (strip, card, etc) it will affect all child components? To make sure child components have proper style?
What if there is a card inside dark strip? .p-strip--dark > .p-card > input
Should input use dark styles (because it's in dark strip), or while card styles? What if we end up in opposite situation (having dark subcomponent inside white strip?). CSS will not handle this by itself based on parent, because there is no way to know what is the "closest" parent background.
Multiply it by all the components with interactive elements: tabs, accordion, side navigation, etc. Now should strip define styles for them? Or should each of them have a list of all possible parents that can affect their background? Neither is perfect.
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.
The problem is not that a component defines backround and not text colour. The problem is that parent compoent may have different theme than child component. So for example input component (that defaults to background working on white) will not look well in dark strips or in paper background, same with tabs, or accordion. Because they all default to white backgrounds.
And there is no easy way for parent component theme to be inherited by children. So when a parent changes background (uses different theme) child components need to use the same theme. But it can't be done automatically via CSS based on parent. For reliability it needs to be via a class name set on the component itself.
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 am sure I'm missing a lot here but could it not be handled at the theme. What is now the default theme of the background? Is it still white and we are adding a is-paper
to the body to switch to grey? What about styling in the form of:
.p-input {
background: grey;
}
.is-paper .p-input {
background: white;
}
.p-card {
background: white;
.p-input {
background: grey;
}
}
I am less concerned if paper is not the default but it would be a pain for the authors of entire pages of mark up to keep adding is-paper
to elements due to a global page theme.
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.
The issue with the example above is that it doesn't scale. It works in a simple case like this, but there may be cases of mode complex nesting. And more importantly there are more effected components than just inputs. There are multiple components that require multiple colour/style changes (default/hover/active) on various elements.
It's also an issue of separation of concerns.
For the code like this:
.p-card { // this has to be done for all possible containers that change background to white
background: white;
.p-input {
background: grey;
}
// add .p-tabs, .p-accordion, … :hover, :active and many more detailed styles here…
}
Which component defines it? Is it a card that defines this (for all possible components that are affected)? Or should an input be aware that inside a card it looks differently? Both of the approaches create dependencies between components in a form of one-to-many or even many-to-many relations (assuming we have multiple components that may have white background, and multiple components affected by this).
And if we implement it this way it relies on CSS specificity and source order to work. If multiple components redefine the theme of their children the one that is last in source order (or with more specific selectors) will end up defining the theme, not the closest parent in HTML.
I know this is not ideal with additional class name, but it's reliable and scoped to the component itself.
This PR is against 4.0 branch, so it is not going live yet. We may have a chance to improve it before final release.
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.
The only compromise I can see that would be relatively reliable and hopefully not too annoying is doing it other way around and having .is-paper .on-white
in stead of .on-paper
.
// assumed default white background of the page
input { background: grey }
// if page is paper themed use proper colour by default
.is-paper input { background: darker-grey }
// if you ever put an input on page that uses "paper"
// inside anything with white background, you need to add class to the input
.is-paper input.on-white { background: grey }
This way there is no direct connection between specific components. Input doesn't need to know which components change background. It just uses the page default.
But if developer uses a component with different background than page default, that's when they need to add a class to input. Which should hopefully be rare.
Would that be better @ClementChaumel @anthonydillon ?
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.
Well, while it's likely not a resolved issue I experimented a bit and it seems that we don't have to introduce any new class names just yet.
We can make inputs adjust to page having paper theme (.is-paper input
) and we don't have to adjust it for white background (because by coincidence it's using transparent colours that will work on white background as well). It may not be a final solution, so we have to watch that space, but at least we don't have to do any changes yet.
It's easier to add some new class names later than remove them.
Co-authored-by: ClementChaumel <clement.chaumel@gmail.com>
Done
Adds paper background option to body.
is-paper
class name on body to change the background to new colour.Fixes WD-3102
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
[if relevant, include a screenshot or screen capture]