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

Create spright-components package #1986

Merged
merged 16 commits into from
Apr 22, 2024
Merged

Conversation

m-akinc
Copy link
Contributor

@m-akinc m-akinc commented Mar 29, 2024

Pull Request

🀨 Rationale

Resolves #1974

πŸ‘©β€πŸ’» Implementation

  • Created packages/spright-components
    • has spright-accordion component with unit tests
    • README and CONTRIBUTING

Configuration includes some Storybook-related content (just to avoid having to remove and re-add later), but the actual Storybook support for spright-components will be a separate submission.

πŸ§ͺ Testing

Builds and tests pass

βœ… Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@m-akinc m-akinc requested a review from jattasNI April 11, 2024 18:55
@m-akinc
Copy link
Contributor Author

m-akinc commented Apr 11, 2024

@jattasNI Not sure it makes sense for someone other than you or Milan to buddy this, so I thought you could take an initial look before having other people review.

@m-akinc m-akinc force-pushed the users/makinc/spright-components branch from cfacfe9 to dbb6edf Compare April 18, 2024 18:09
@m-akinc m-akinc requested a review from jattasNI April 18, 2024 21:10
@m-akinc m-akinc marked this pull request as ready for review April 18, 2024 22:38
@m-akinc m-akinc requested a review from rajsite as a code owner April 18, 2024 22:38
packages/spright-components/CONTRIBUTING.md Outdated Show resolved Hide resolved
packages/spright-components/CONTRIBUTING.md Outdated Show resolved Hide resolved
packages/spright-components/CONTRIBUTING.md Outdated Show resolved Hide resolved
packages/spright-components/README.md Outdated Show resolved Hide resolved
packages/spright-components/README.md Show resolved Hide resolved
packages/spright-components/src/demo/template.ts Outdated Show resolved Hide resolved
m-akinc and others added 3 commits April 19, 2024 11:16
Co-authored-by: Jesse Attas <jattasNI@users.noreply.github.com>
@m-akinc m-akinc requested a review from rajsite April 19, 2024 21:18
Co-authored-by: Milan Raj <rajsite@users.noreply.github.com>
@m-akinc m-akinc requested a review from rajsite April 19, 2024 22:12
@m-akinc m-akinc requested a review from rajsite April 19, 2024 23:30
Copy link
Member

@rajsite rajsite left a comment

Choose a reason for hiding this comment

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

Waiting for comments to be addressed

"@microsoft/fast-element": "^1.12.0",
"@microsoft/fast-foundation": "^2.49.6",
"@microsoft/fast-web-utilities": "^6.0.0",
"@ni/nimble-components": "*",
Copy link
Member

Choose a reason for hiding this comment

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

This is not a minor change... in a published package that would mean any version of nimble-components is valid which is not accurate. You just need to get it aligned with what is the latest in main and synced to your branch. Beachball handles revving it and keeping it aligned after.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I had been thinking of "*" as "latest", since it allows the newest version, but of course that's not what it means. Switched it back.

Copy link
Member

Choose a reason for hiding this comment

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

"latest" does exist but we should use semantic version ranges to specify the version we actually test against. i.e. latest changes day-to-day and doesn't care about major versions, etc. If you want to make sure the mental model for how semver works is aligned we should discuss offline as you're setting up more packages.

Copy link
Member

Choose a reason for hiding this comment

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

This came up in the wild for a third party package using wildcard specifiers for dependencies as an example: DefinitelyTyped/DefinitelyTyped#69355 (comment)

Even though the linkify-it package did a major version release to specify a breaking change users of markdown-it (directly or indirectly) were broken by the major change because markdown-it used a wildcard specifier.

Copy link
Member

@rajsite rajsite left a comment

Choose a reason for hiding this comment

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

Needs correct package version management

@m-akinc m-akinc requested a review from rajsite April 22, 2024 18:56
@rajsite rajsite merged commit f50a9f1 into main Apr 22, 2024
13 checks passed
@rajsite rajsite deleted the users/makinc/spright-components branch April 22, 2024 21:51
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.

Create spright-components package
3 participants