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

doc(adrs): add theming peer dependency ADR-001 #1560

Merged
merged 5 commits into from
Jun 16, 2023
Merged

Conversation

jzempel
Copy link
Member

@jzempel jzempel commented Jun 14, 2023

Description

Brings the decision made 2023.04.24 re: the @zendeskgarden/react-theming peer dependency into GitHub, along with a README for indexing future ADRs.

@jzempel jzempel requested a review from a team as a code owner June 14, 2023 18:39
@coveralls
Copy link

coveralls commented Jun 14, 2023

Coverage Status

coverage: 96.154%. remained the same when pulling 298f8b0 on jzempel/adr-001 into d1774fd on main.

Copy link
Contributor

@geotrev geotrev left a comment

Choose a reason for hiding this comment

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

Just a few grammar/wording suggestions.

docs/adrs/001-theming-peer-dependency.md Outdated Show resolved Hide resolved
docs/adrs/001-theming-peer-dependency.md Outdated Show resolved Hide resolved
docs/adrs/001-theming-peer-dependency.md Outdated Show resolved Hide resolved
Co-authored-by: George Treviranus <3946669+geotrev@users.noreply.github.com>
Copy link
Contributor

@geotrev geotrev left a comment

Choose a reason for hiding this comment

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

🚀 :shipit:

## Context

Garden provides a collection of React components through multiple NPM packages.
Each package has peerDependencies for `@zendeskgarden/react-theming`, `react`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Each package has peerDependencies for `@zendeskgarden/react-theming`, `react`,
Each package has `peerDependencies` for `@zendeskgarden/react-theming`, `react`,

#nit - if we are camel casing, should we wrap it as inline code?

Comment on lines 18 to 19
to ingest duplicated and more brittle code. A console warning will notify the
consumer if a peer version is out-of-date.
Copy link
Contributor

Choose a reason for hiding this comment

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

By console warning, do you mean in the terminal?

consumer if a peer version is out-of-date.

Ideally, all Garden package versions should align. However, at the very least,
the theming peer dependency should align with the highest version of a Garden
Copy link
Contributor

@Francois-Esquire Francois-Esquire Jun 15, 2023

Choose a reason for hiding this comment

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

Suggested change
the theming peer dependency should align with the highest version of a Garden
the `@zendeskgarden/react-theming` peer dependency should be kept in sync to the latest version.

Suggestion -> Being literal about "theming peer dependency" to remove any room for interpretation, the same with "highest version".

Copy link
Member Author

@jzempel jzempel Jun 15, 2023

Choose a reason for hiding this comment

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

...kept in sync to the latest version

is not technically accurate for what we are asking of the consumer. For example, when Garden v9 lands, we are not necessarily asking the consumer to make sure the theming package is v9. Rather, this ADR is specifying that theming should be in sync with whatever highest version of Garden package the consumer has installed – and, technically, must be up-to-date with whatever highest peer dependency any given Garden package calls for.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay I understand now and fair point.

treatment for `@zendeskgarden/react-theming`.

While this decision conflicts with [semantic versioning](https://semver.org/),
it benefits both maintainers and consumers by enabling centralized and tested
Copy link
Contributor

@Francois-Esquire Francois-Esquire Jun 15, 2023

Choose a reason for hiding this comment

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

Suggested change
it benefits both maintainers and consumers by enabling centralized and tested
it benefits both maintainers and consumers by enabling a centralized, tested collection of hooks and utilities across the codebase.

Suggestion -> The sentence was hard to read..

@jzempel jzempel merged commit e1a2f86 into main Jun 16, 2023
1 check passed
@jzempel jzempel deleted the jzempel/adr-001 branch June 16, 2023 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants