Skip to content
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

Merged
merged 8 commits into from May 26, 2023
Merged

Paper background #4763

merged 8 commits into from May 26, 2023

Conversation

bartaz
Copy link
Contributor

@bartaz bartaz commented May 10, 2023

Done

Adds paper background option to body.

  • Adds is-paper class name on body to change the background to new colour.
  • Adjusts colours of inputs (and search box) when used on pages with paper background

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:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
  • Vanilla version in package.json should 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

[if relevant, include a screenshot or screen capture]

@webteam-app
Copy link

Demo starting at https://vanilla-framework-4763.demos.haus

@bartaz bartaz mentioned this pull request May 10, 2023
3 tasks
@bartaz
Copy link
Contributor Author

bartaz commented May 10, 2023

Recreated from #4747

renovate bot and others added 5 commits May 17, 2023 13:19
@bartaz bartaz changed the base branch from main to vanilla-4.0 May 17, 2023 11:20
@bartaz
Copy link
Contributor Author

bartaz commented May 17, 2023

Edited to point at vanilla-4.0 branch, it's too big of a change (affecting themes and other components) to do it in half baked state in Vanilla 3.

Copy link
Contributor

@ClementChaumel ClementChaumel left a 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">
Copy link
Contributor

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

Copy link
Contributor Author

@bartaz bartaz May 23, 2023

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@bartaz bartaz May 23, 2023

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 ?

Copy link
Contributor Author

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.

templates/docs/base/paper.md Outdated Show resolved Hide resolved
releases.yml Outdated Show resolved Hide resolved
templates/docs/base/paper.md Outdated Show resolved Hide resolved
templates/docs/base/paper.md Outdated Show resolved Hide resolved
bartaz and others added 2 commits May 24, 2023 12:41
Co-authored-by: ClementChaumel <clement.chaumel@gmail.com>
@bartaz bartaz merged commit af7e1cc into vanilla-4.0 May 26, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants