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

Build on top of salt doc updates #4054

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mark-tate
Copy link
Contributor

@mark-tate mark-tate commented Sep 4, 2024

Closes #3847

Copy link

vercel bot commented Sep 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
saltdesignsystem ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 5, 2024 11:15am

Copy link

changeset-bot bot commented Sep 4, 2024

⚠️ No Changeset found

Latest commit: 7010902

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

origami-z and others added 4 commits September 4, 2024 21:56
… shared responsibility

- provided an overview of the options to theming, which is intended to link out to other docs
- re-formatted as a set of do's and don't
Copy link
Contributor

@origami-z origami-z left a comment

Choose a reason for hiding this comment

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

Rewrite of site/docs/theming/best-practices.mdx introduces a few new guidelines doesn't existed before, which needs a lot more team-wide review.

@@ -23,6 +29,18 @@ For a full list of foundational components, refer to the [component documentatio

A design token is a visual attribute that represents a specific aspect of a design system. It is an abstract representation of a design property, such as colors, typography, spacing, or breakpoints, that can be used consistently across a product or design system. Design tokens provide a centralized and reusable way to define and manage design-related values, making it easier to maintain consistency and make global design changes. They act as a bridge between designers and developers, enabling a shared language and ensuring design consistency throughout the product or system. In Salt design tokens are represented as Figma variables and in code, they take the form of CSS variables.

## Foundation Layer

The foundation layer is the lowest layer of the Salt theme, consisting of primitive values that should remain stable between releases. Ideally, it should not be referenced directly but through the palette and characteristics layers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Spacing and size values will be referenced here. Remove last sentence

@@ -35,6 +53,6 @@ Salt is the J.P. Morgan design system, an open-source solution for building exce

## Theming

Salt is under active development. At this stage, we don't recommend theming unless absolutely necessary.
Refers to the process of customizing the visual appearance of a user interface (UI) by altering predefined design elements such as colors, typography, spacing, and other stylistic attributes. Theming allows for the creation of a cohesive and consistent look and feel across an application, ensuring that it aligns with brand guidelines and user experience (UX) standards.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add only supported by salt next ?

@@ -0,0 +1,140 @@
---
title: Best practices for theming
Copy link
Contributor

Choose a reason for hiding this comment

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

Building custom components and theming are 2 different areas, that's the reason why it's hard to get a good title

- Do use the Salt theme "out of the box" and avoid owning any custom styles.
- Do recognize that adopting Salt aligns your current UX with a maintained standard that conforms to both design and accessibility guidelines. Whilst there is an opportunity cost, deviating from Salt can increase your costs and result in a disparate UI, ultimately affecting the end user's experience.

### Option 2: Compose your theme from pre-made design options
Copy link
Contributor

Choose a reason for hiding this comment

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

1&2 shouldn't be split up, otherwise it could be seen as 2 different level of customization


### Option 3: Extend the Salt theme

Each Salt theme consists of 3 layers ([Foundation](/salt/about/glossary#foundation), [Palette](/salt/about/glossary#palette-layer) and [Characteristics](/salt/about/glossary#characteristics-layer) layers) that work together to offer a consistent theming solution across all components:
Copy link
Contributor

Choose a reason for hiding this comment

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

These 2 paragraphs are not defining what is "Extend the Salt theme"?


Whilst this creates the highest level of flexibility it comes with an increased level of responsibility to maintain. If this is what you need , then here are our recommendations:

#### Don'ts
Copy link
Contributor

Choose a reason for hiding this comment

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

Dos and Don't tells a few different opinions from Salt, needs very careful team-wide review

- Do change the characteristics layer but federate the solution to a central theme, ensuring that deviations from the Salt standard are known and can be easily updated if required.
- Do review the Salt backlog/roadmap before making a local change, to understand whether your change is tactical or strategic

We've documented all our [design tokens](/salt/theming/index#design-tokens), but let's start with some basic tokens that have the biggest effect:
Copy link
Contributor

Choose a reason for hiding this comment

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

let's start with some basic tokens that have the biggest effect

This feels the encouragement of changing these values?


<br />
These four variables represent key aspects of theming controls: background,
foreground, typography, and border color. Exploring these variables will
Copy link
Contributor

Choose a reason for hiding this comment

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

Exploring these variables

We shouldn't encourage change these

@origami-z
Copy link
Contributor

origami-z commented Sep 23, 2024

This adds too many new recommendations to the original intend of basic educational purpose, which means eating into product roadmap and #4004. I think we should close this entirely and rework piece by piece.

My original page intentionally avoided overlapping with product roadmap or the main themes page, hoping to get easier review. With the current format, it's impossible to split and comment.

@origami-z
Copy link
Contributor

Back to draft while working out #3847

@origami-z origami-z marked this pull request as draft September 26, 2024 09:16
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.

Document how to build a custom component using Salt theme
2 participants