Initial structure for shared website styles #231

parlough · 2024-10-09T23:19:52Z

Overview

This PR proposes a general structure for hosting shared styles in this repository. Currently this is primarily CSS custom property (variable) declarations. It is not yet intended to be used by downstream sites, but rather for discussion and determination of what variables can be shared, as well as their values. For now, anything in the package and these files can and will break without notice.

Proposed structure

The following is a rough initial structure that I came up with. Feel free to suggest large changes or make changes after this lands.

pkgs/dash_design
A package to contain the styles. They can be depended on by manually copying relevant files or by depending on the dash_design package (with a path and/or git dependency for now).
- lib/styles.scss
  A file that can be used directly with all current variables/classes already applied. This applies styles on the :root and applies dark mode if data-theme=dark on <html>. By default this doesn't include Google-specific styles (such as Google Sans), so needs to be configured when used or forwarded:
```
@forward 'package:dash_design/styles' with (
  $google-site: true
);
```
  This file is also used to test the styles, to make sure they can be built by Dart Sass.
- lib/styles
  If you need more fine grained control, such as on what element dark mode variables are specified, you can use/include the mixins provided by files in this directory.
  - lib/styles/variables.scss
    Declares mixins that declare relevant CSS custom properties (variables). Currently shared, light-theme, dark-theme, and google-overrides. If needed, this can be broken up across different files in the future, into sub-mixins, whatever works best. The first declaration of each variable should contain a comment about the variable and some examples of when it is to be used.

Open questions and TODOs

Does this structure make sense? If it were to actually be used, would it work for the downstream sites?
Has pub.dev determined a naming scheme for CSS variables that works well or they prefer? Is there a larger standard we should follow? Same goes for mixins. Document our initial decision before landing this, but we should feel free to change if needed.
Does relying on data-theme=dark on the HTML element work for pub.dev? Or would a class on the body be better?
As far as I can tell, Sass doesn't support doc comments. So I used Dart's convention of triple backslashes (///) for doc comments and double (//) for inline comments. Does that make sense or am I missing a different standard?
Should we make all Sass deprecation warnings fatal for CI?
Is there any standard SCSS formatter (or one you like)?
Anything else?

\cc @isoos @jonasfj

Regenerate workflow

isoos · 2024-10-11T08:31:39Z

pkgs/dash_design/lib/styles/variables.scss

+/// CSS custom properties (variables) to be used for a light theme.
+@mixin light-theme {
+  /// The default text color, used for body text.
+  --dash-on-background-text-color: #212121;


Note: pub.dev uses #4a4a4a and that is a step down in lightness from 29% to 13%. Not sure: what should be the process for larger changes like this?
/cc @jonasfj @sigurdm

I would have to see it on a staging site or screenshot to really have any opinion here.

I'll update this PR to use #4a4a4a for the purposes of this PR getting an initial structure in place.

I'll need to look back at why we moved away from it on the Dart site. It might be a bit too soft and low contrast (≈9 vs ≈16) for long text. Perhaps there's something in the middle that feels right, or body text could use a separate, higher contrast color?

pkgs/dash_design/lib/styles/variables.scss

pkgs/dash_design/lib/styles.scss

isoos · 2024-10-11T08:46:43Z

pkgs/dash_design/lib/styles.scss

+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+/// styles.scss:


Is there any standard SCSS formatter (or one you like)?

We should investigate if package:sass_api could be used for something like that.

I think if it would be part of the Dart toolchain, nobody would mind it, however, if it is IDE-based or required extra setup, it would cause a lot more friction.

I started investigating this and if it works out well, I'll open a follow-up PR to add it and get feedback.

I'm sure it's possible to build one on-top of analyzer_plugin using sass_api -- but it might also be a somewhat large project 🤣

pkgs/dash_design/lib/styles.scss

isoos · 2024-10-11T08:57:28Z

pkgs/dash_design/lib/styles/variables.scss

+/// CSS custom properties (variables) to be used for a light theme.
+@mixin light-theme {
+  /// The default text color, used for body text.
+  --dash-on-background-text-color: #212121;


Has pub.dev determined a naming scheme for CSS variables that works well or they prefer? Is there a larger standard we should follow? Same goes for mixins. Document our initial decision before landing this, but we should feel free to change if needed.

I can be convinced to use any convention, as long as the decision how to use and extend it is straightforward, and reading it is not confusing. Couple of further notes and requirements that I have collected so far:

The variable names should be unique and never be a prefix of another variable. This makes searching, usage check and replacing straightforward with regular text tools.

The variable names should be short.

I have found the reading of mixed _ and - a bit harder than - and lowerCamelCase, now in the process of migrating pub.dev to it.

For role names like neutral, inset, selection I've found that this list provides good starting points, although they have this reversed naming scheme which we haven't applied: https://primer.style/foundations/primitives/color
I see some usefulness for the reversed naming, but I couldn't really tell which one has more benefits.

I think @sigurdm also has some preferences here.

No I really don't have preferences here.

I rarely edit the css.

Thanks for the insights, I think that all sounds good to try out. The harder part seems to be determining the role names :P

The variable names should be unique and never be a prefix of another variable. This makes searching, usage check and replacing straightforward with regular text tools

I think this generally makes sense, but what about variants and modifiers? For example, the primer examples have interaction state after the color type (--button-default-bgColor-selected), and my initial instinct is I like that. That way when I'm working on a component in my IDE and I know my desired root color, I can use the same prefix to determine what other variants/states are supported.

isoos · 2024-10-11T08:59:58Z

pkgs/dash_design/lib/styles/variables.scss

+/// into any desired scope.
+
+/// CSS custom properties (variables) to be used across all themes.
+@mixin shared {


Does this structure make sense? If it were to actually be used, would it work for the downstream sites?

I think we mostly start with the color mixins. I can imagine a future where we have a fully shared CSS rule structure, but it could be a separate effort from the color schemes.

Yeah that makes sense and sounds good to me! I'll keep the lib/styles.scss file updated for the purpose of having a file to test with and have a structure in mind, but the mixins it includes can be used manually for now :)

sigurdm · 2024-10-11T13:41:24Z

Before

After

The darker font somehow looks "harsh" to me. Maybe it is a matter of getting used to it, but I think I prefer the old color.

jonasfj · 2024-10-18T12:12:22Z

Just my two cents...

I think that lib/styles/variables.scss makes a lot of sense.
This allows us to share colors, fonts and things like that.

I'm a little more worried about styles. Because if you import styles it's harder to check what changed, and whether or not anything broke. But I also see the point that the things we style here aren't rocket science.

Experiment with a structure for shared styles

37aea4e

Regenerate workflow

parlough force-pushed the feat/dash-design-package branch from bd33613 to 37aea4e Compare October 10, 2024 02:21

parlough changed the title ~~Experiment with a structure for shared styles~~ Initial structure for shared website styles Oct 10, 2024

Add template for a README

3cb6eaf

parlough requested a review from isoos October 10, 2024 17:18

isoos reviewed Oct 11, 2024

View reviewed changes

parlough added 6 commits October 18, 2024 00:02

Merge branch 'main' into feat/dash-design-package

1fc526d

Separate typography from colors

3858a28

Run the build test with all deprecations enabled

384c8c1

Switch to using dark-theme on body for now

3b5060f

Adjust naming scheme

029a2f6

Start with current pub text colors

f532780

jonasfj approved these changes Oct 23, 2024

View reviewed changes

isoos approved these changes Oct 23, 2024

View reviewed changes

isoos merged commit 99868e7 into main Oct 23, 2024
8 checks passed

isoos deleted the feat/dash-design-package branch October 23, 2024 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial structure for shared website styles #231

Initial structure for shared website styles #231

parlough commented Oct 9, 2024 •

edited

Loading

isoos Oct 11, 2024

sigurdm Oct 11, 2024

parlough Oct 18, 2024 •

edited

Loading

isoos Oct 11, 2024

parlough Oct 18, 2024

jonasfj Oct 18, 2024

isoos Oct 11, 2024

sigurdm Oct 11, 2024

parlough Oct 18, 2024

isoos Oct 11, 2024

parlough Oct 18, 2024 •

edited

Loading

sigurdm commented Oct 11, 2024

jonasfj commented Oct 18, 2024

Initial structure for shared website styles #231

Initial structure for shared website styles #231

Conversation

parlough commented Oct 9, 2024 • edited Loading

Overview

Proposed structure

Open questions and TODOs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parlough Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parlough Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

sigurdm commented Oct 11, 2024

jonasfj commented Oct 18, 2024

parlough commented Oct 9, 2024 •

edited

Loading

parlough Oct 18, 2024 •

edited

Loading

parlough Oct 18, 2024 •

edited

Loading