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

docs: add section on token definition structure #1671

Merged
merged 6 commits into from
Apr 18, 2022

Conversation

hbuchel
Copy link
Contributor

@hbuchel hbuchel commented Apr 12, 2022

Description of changes

We don't have an explicit structure defined for creating component tokens for the web and there are a couple different patterns used throughout our components, currently. We should have this defined so that contributors know how to structure a component's tokens to be consistent with those already defined, and so that users of the tokens have a consistent way to discover what tokens are available.

After a brief chat with @dbanksdesign this is the proposal:

component[variant][state][child]

I included a pseudo-code version of what that looks like in practice in this PR for updating the Theming docs.

This allows us to keep each definition type grouped. This means you could readily find all of a particular state's tokens for a variation without hunting through nested child objects. That is:

export const component = {
  childA: { /* tokens */ },
  childB: { /* tokens */ },
  _focus: {
    childA: { /* tokens */ },
    childB: { /* tokens */ }
  },
  large: {
    childA: { /* tokens */ },
    childB: { /* tokens */ },
    _focus: {
      childA: { /* tokens */ },
      childB: { /* tokens */ },
    }
  }
}

As opposed to:

export const component = {
  childA: {
    /* tokens */
    _focus: { /* tokens */ }
  },
  childB: {
    /* tokens */
    _focus: { /* tokens */}
  },
  large: {
    childA: {
      /* tokens */
      _focus: { /* tokens */ }
    },
    childB: {
      /* tokens */
      _focus: { /* tokens */}
    },
  }
}

These are simple examples, but for larger components that have many child elements and tokens, the second version might require a little more sleuthing around the file to find all of the affected, in this example, focus styles for a component.

Really, the most important part is aligning on one format so that developers know how to proceed when making new definitions, and so users can have a consistent reading and discoverability experience for the token files.

This also calls out the use of an underscore prefix for defining state tokens, which helps with quickly distinguishing a state from a variation name. That is, we should do:

_focus: {},
_hover: {},
_disabled: {},

and not

focus: {},
hover: {},
disabled: {},

Open Question

  • Is this the best place for this to live, on the Theming overview page? The Theming section in our docs is aimed more at the consumers of the library; while this information is a little more aimed at developers who want to contribute to Amplify UI itself. I could see where it's useful for consumers, though, as it's nice to know what to look for when learning what tokens are available.

Issue #, if available

N/A

Description of how you validated changes

Tested in local docs instance.

Checklist

  • PR description included
  • yarn test passes
  • Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@changeset-bot
Copy link

changeset-bot bot commented Apr 12, 2022

⚠️ No Changeset found

Latest commit: f68eaa4

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

Following this pattern, our token definition for the component would follow this general structure. **Note**: Amplify UI prefixes states with an underscore, `_`, to help distinguish state names from variation names.

```javascript
export const component = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like it might be more useful if we provide a real example from the codebase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1, will update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to include an abridged example (didn't want to include the whole file) from the Button component.

Copy link
Contributor

@reesscot reesscot left a comment

Choose a reason for hiding this comment

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

I like the proposal, just had some comments on aiming it more towards users of the theme, rather than contributors. Users do have to know how to reach specific component tokens if they want to define their own theme.

docs/src/pages/theming/theming.web.mdx Outdated Show resolved Hide resolved
docs/src/pages/theming/theming.web.mdx Outdated Show resolved Hide resolved
hbuchel and others added 2 commits April 13, 2022 11:56
Co-authored-by: Scott Rees <6165315+reesscot@users.noreply.github.com>
Co-authored-by: Scott Rees <6165315+reesscot@users.noreply.github.com>
Copy link
Contributor

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

Looks good, just one question from me and I like Scott's idea for a more concrete example.

Amplify UI follows a consistent pattern when defining tokens for a component's states and variations. This is helpful for discovering what tokens are available for theming different aspects of a component. Amplify UI uses the following pattern:

```javascript
component[variant][_state][child];
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change [variant] to [modifier]? Some components have a variant prop as well as a size prop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works for me; maybe a little more inclusive of both variants (primary, link, etc.) and size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to include this change, and added some definitions for those terms (modifier, state).

@hbuchel hbuchel temporarily deployed to ci April 15, 2022 17:42 Inactive
@hbuchel hbuchel temporarily deployed to ci April 15, 2022 17:42 Inactive
@hbuchel hbuchel temporarily deployed to ci April 15, 2022 17:42 Inactive
@hbuchel hbuchel temporarily deployed to ci April 15, 2022 17:42 Inactive
@hbuchel hbuchel merged commit 1ccbc17 into main Apr 18, 2022
@hbuchel hbuchel deleted the docs/theming-nesting-tokens branch April 18, 2022 14:10
zchenwei added a commit that referenced this pull request Apr 20, 2022
* Added new React Guide for Authenticated Routes (#1675)

* Began work on new guides for auth

* Added React guide

* Updated guide

* capitilization update

* Addex Example Wrapper

Co-authored-by: Erik Hanchett <ehhanche@amazon.com>

* chore: align typescript version at root devDependencies (#1686)

* Fixed issue with vue examples (#1688)

Co-authored-by: Erik Hanchett <ehhanche@amazon.com>

* chore: use tsup for dev build (#1690)

* feat(Authenticator): accept general JSX children (#1685)

* Allow non-function children in Authenticator

* Create six-bikes-develop.md

* Update six-bikes-develop.md

* Update packages/react/src/components/Authenticator/Router/index.tsx

Co-authored-by: Caleb Pollman <cpollman@amazon.com>

* Update packages/react/src/components/Authenticator/Router/index.tsx

* Use React.ReactNode and consolidate children type

* Reduce nested ternary

* Add fragments around children

* Fix comments

* Update .changeset/six-bikes-develop.md

Co-authored-by: Caleb Pollman <cpollman@amazon.com>

Co-authored-by: Caleb Pollman <cpollman@amazon.com>

* fix: update focus styling for switchField  (#1667)

* fix: update focus styling for switchField

Co-authored-by: Heather Buchel <buchel@amazon.com>

* chore: update required permissions for publish-latest (#1708)

* docs: add section on token definition structure (#1671)

* docs: add section on token definition structure

Co-authored-by: Heather Buchel <buchel@amazon.com>
Co-authored-by: Scott Rees <6165315+reesscot@users.noreply.github.com>

* chore: (docs) update tabs component demo (#1703)

* chore: (docs) update tabs component demo

Co-authored-by: Heather Buchel <buchel@amazon.com>

* fix(docs Authenticator): Update style authenticator (#1707)

* Updated docs for styling for authenticator

* fixed it so only React sees information about provider

Co-authored-by: Erik Hanchett <ehhanche@amazon.com>

* chore: updating docs build to respect yarn.lock (#1714)

* chore: updating docs build to respect yarn.lock

* chore: adding resolution to clear cache

* Update tests.yml (#1715)

* chore: removing dep resolution

Co-authored-by: William Lee <43682783+wlee221@users.noreply.github.com>

* docs: fixing controlled example bug (#1716)

* docs: fixing dark mode example (#1719)

* docs: update Pagination (#1695)

* refactoring main pagination demo

* more pagination examples

* pagination styling examples

* adjust styles

* demonstrate usePagination hook, and include Theme example for styling

* remove extra backticks

* remove empty code section

Co-authored-by: Joe Buono <joebuono@amazon.com>

* chore: update menu demo to new format (#1702)

* fix(docs): Fix anchor tags between pages (#1706)

* Split off tests to `test-main` and `test-prs` (#1697)

* Split off tests to `test-main` and `test-prs`

* Adjust workflow names

* Add comments

* Skip cache for docs

* chore(ui-react): add jest generated coverage directory to .gitignore (#1718)

* chore: Bump maplibre-gl-js-amplify dependency version (#1721)

* Bump maplibre-gl-js-amplify dependency version

* Create patch changeset for version bump

* chore(bump map libre) (#1722)

* Bump maplibre

* Added changeset

* Added yarn lock

Co-authored-by: Erik Hanchett <ehhanche@amazon.com>

* docs: updating Link docs (#1709)

* docs: updating Link docs

* chore: addressing comments

* docs(fix): remove Theme tab from demos (#1723)

* temporarily remove Theme tab from docs demos

* removed nullish coalescing operator

* small typo

Co-authored-by: Joe Buono <joebuono@amazon.com>

* chore(ui-react): type withAuthenticator (#1724)

* chore: fixing workflow not publishing to next tag (#1725)

* chore: fixing workflow not publishing to next tag

* chore: updating workflow

* chore: updating publish-geo

* chore: update view demo to new format (#1710)

* Version Packages (#1693)

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* docs: updating placeholder docs (#1705)

* docs: updating placeholder docs

* docs: adding Theme section

* chore: addressing comments

Co-authored-by: Erik Hanchett <icystorm@gmail.com>
Co-authored-by: Erik Hanchett <ehhanche@amazon.com>
Co-authored-by: Scott Rees <6165315+reesscot@users.noreply.github.com>
Co-authored-by: William Lee <43682783+wlee221@users.noreply.github.com>
Co-authored-by: Caleb Pollman <cpollman@amazon.com>
Co-authored-by: Heather Buchel <hbuchel@gmail.com>
Co-authored-by: Heather Buchel <buchel@amazon.com>
Co-authored-by: Joe Buono <joebuono724@gmail.com>
Co-authored-by: Joe Buono <joebuono@amazon.com>
Co-authored-by: Shane Laymance <shane.laymance@gmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.

4 participants