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

Add basic clean-theme support and update card, frame patterns #89

Merged
merged 1 commit into from
Jun 15, 2021

Conversation

lyzadanger
Copy link
Contributor

@lyzadanger lyzadanger commented Jun 3, 2021

This PR solves #85 by:

  • adding a theme mixin that other mixins can use to apply theme-specific styles
  • Updating the card and frame mixins to theme for the clean theme

I'm hoping that the changes themselves will obviate further explanation here (that's the goal here, anyway).

To finish #85, after #88 is resolved, we'll want to go back in and see how the modal components look in a clean theme. We may wish, e.g., to apply borders to modals even in the clean theme. (This is done)

Fixes #85

@include atoms.border;
@include layout.vertical-rhythm;

padding: $-space;
border-radius: $-border-radius;
background-color: $-color-background;

@include themes.theme('clean') {
// A frame should not have any borders in the clean 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.

Both this comment and the one further down in this module are "x is 5"-type comments, but the point I'm trying to make is that we should document theme deviations and their intent.

@@ -15,13 +16,18 @@ $-space: var.$layout-space;
/**
* Give an element a border, background color and internal vertical spacing
*/
@mixin frame($with-hover: false) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Derp. This argument was unused.

Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "clean" theme mainly exists to satisfy the needs of a specific customer in the client. We may need to have it in the pattern library for that reason but we probably don't want to encourage its use elsewhere. Perhaps this is just something that can be noted in the docs. I wonder whether this mechanism could potentially support something more generally useful like a dark theme in future?

@lyzadanger
Copy link
Contributor Author

lyzadanger commented Jun 4, 2021

The "clean" theme mainly exists to satisfy the needs of a specific customer in the client.

And don't I know it! And there's more, because to get the intended full effect of the clean theme, you also need to configure the client with several branding configuration properties.

I'm going to think about this a little more. Maybe this customization belongs entirely in the client. There are kind of two problems involved here:

  • Support for theming, in general. This approach would move toward that, but we don't need to solve that problem yet, necessarily.
  • Support for this specific, customer-related theming which only applies to the Sidebar (and in a couple of places, the Notebook). That needs to be solved before we can use shared Dialogs/Panels/Modals in the client, but doesn't need to be solved here necessarily.

On a tactical level, I wonder if this package should aim to keep all non-component specificity at (0, 0, 1, 0) or lower (that translates to a single class selector), and leave higher specificity to applications that use it. That may be too extreme, but it's a thought...

@lyzadanger
Copy link
Contributor Author

After some soul-searching and reflection, I would like to opt to move forward with this approach to theming, in which this package applies some clean-theme styles.

The “clean” theme is only relevant to the client application, true.

But there’s no way to implement the theming affordances for the client-only clean theme without leakage about that theming in some way:

  • With this approach, this project’s package has to “know” about the clean theme and provide styling affordances for it.
  • If we did the clean-theming in the client project, however, the client would have to “know” about the package enough to extend classes or mixins. And getting the theming right is easier if a lower-level mixin (in this case, the card and frame patterns) are themed. I don’t especially want to make those mixins part of the public API, especially not now.

In looking at those two, the first (this PR’s direction) seems less problematic to me at present, additionally because:

  • The overall amount of stying needed to effect the clean theme is fairly minimal (there just isn’t that much of it). So, yes, this package knows a little about the clean theme, but it’s not a huge pile.
  • It will centralize theming. Say we introduce a dark mode or something. We’ll be able to implement it with the same theming-mixin pattern and see how the two (dark vs. clean) play together (or don’t).
  • No changes are required in the applications (i.e. client) to use the components and patterns; they just work out of the box.

A side note about Modals: Although Modals are styled with the card and frame patterns (through multi-level inheritance), which have clean-theme affordances, they will not apply clean-theme styles in the client when the clean theme is activated. That is, the Modal components look the same in the client whether or not clean theme is activated. This is actually fine, as modals look better with a border, even in the clean theme. The reason the styles aren’t picked up is that Modals are attached to document.body when they’re rendered, and are not inside the application container that has the theme-clean class. tl;dr no user-facing changes will arise when these changes are released because the one shared component in current use that this would ostensibly affect is not affected.

@codecov
Copy link

codecov bot commented Jun 15, 2021

Codecov Report

Merging #89 (a421e98) into main (24cad39) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #89   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          121       121           
  Branches        36        36           
=========================================
  Hits           121       121           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24cad39...a421e98. Read the comment docs.

@lyzadanger lyzadanger requested a review from robertknight June 15, 2021 13:12
@robertknight
Copy link
Member

The reason the styles aren’t picked up is that Modals are attached to document.body when they’re rendered, and are not inside the application container that has the theme-clean class.

This is potentially a little brittle. We can probably accept the risk for the moment, but if we can find a way to more directly enforce the non-usage of the "clean" theme inside modals that might prevent accidental breakage in future.

@lyzadanger
Copy link
Contributor Author

This is potentially a little brittle. We can probably accept the risk for the moment, but if we can find a way to more directly enforce the non-usage of the "clean" theme inside modals that might prevent accidental breakage in future.

I'm working on replacing Panel components in the client right now with the shared variant, and will address this in that work (it sounds unrelated, but it actually is).

Add a theming mixin to allow other mixins to define rules that should
apply for a given theme.

Add some affordances to the `card` and `frame` molecule patterns for
the `clean` theme and update pattern-library examples.
@lyzadanger lyzadanger merged commit 6dc83d6 into main Jun 15, 2021
@lyzadanger lyzadanger deleted the clean-theme-cards branch June 15, 2021 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement theme-clean variants of card pattern
2 participants