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

ci: Setup for project references #919

Merged
merged 98 commits into from
Jan 11, 2021
Merged

Conversation

ActuallyACat
Copy link
Contributor

@ActuallyACat ActuallyACat commented Jan 7, 2021

Objective

This PR moves all stories to their corresponding package and ensures the stories are not bundled when publishing.

Motivation and Context

This is a task in my housekeeping list and is something (comparatively) small that I could slice out of the project references work: https://cultureamp.atlassian.net/wiki/spaces/DesignSystem/pages/1924630820/Kaizen+Housekeeping

Checklist

[ ] If this contains visual changes, has it been reviewed by a designer?
[ ] I have considered likely risks of these changes and got someone else to QA as appropriate
[ ] I have or will communicate these changes to the front end practice

@ActuallyACat ActuallyACat changed the title Ally/setup for references ci: Setup for project references Jan 8, 2021
@@ -0,0 +1,134 @@
/* eslint import/no-extraneous-dependencies: 0 */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, since this is a story and not something that is to be distributed, I could have linked to this file that is also present in draft-packages/hierarchical-menu/docs/hierarchicalStoriesHelpers.ts. It felt like code smell / something that would encourage bad practice so I have duplicated this file instead.

Copy link

@lijuenc lijuenc Jan 10, 2021

Choose a reason for hiding this comment

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

Ah ok, so with this new structure, every package's stories should be self-contained?

In this case, hierarchical-select uses hierarchical-menu internally. So we're saying it's ok to reference other packages for implementation code, but not for the stories.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, pretty much. The stories are not within the distributed package so one would have to import using a relative link.

@@ -9,3 +9,4 @@ lerna-*.log
/packages/**/*.js.map
/packages/**/*.d.ts
.DS_Store
dist
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the future, all distributed assets will belong in a dist directory instead of being compiled in place.

@ActuallyACat ActuallyACat marked this pull request as ready for review January 8, 2021 07:18
@ActuallyACat ActuallyACat requested a review from a team as a code owner January 8, 2021 07:18
Copy link

@lijuenc lijuenc left a comment

Choose a reason for hiding this comment

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

moves all stories to their corresponding package

Makes a lot of sense.

@@ -0,0 +1,134 @@
/* eslint import/no-extraneous-dependencies: 0 */
Copy link

@lijuenc lijuenc Jan 10, 2021

Choose a reason for hiding this comment

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

Ah ok, so with this new structure, every package's stories should be self-contained?

In this case, hierarchical-select uses hierarchical-menu internally. So we're saying it's ok to reference other packages for implementation code, but not for the stories.

@ActuallyACat ActuallyACat merged commit e681900 into master Jan 11, 2021
@ActuallyACat ActuallyACat deleted the ally/setup-for-references branch January 11, 2021 03:46
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.

2 participants