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

Configuration, reorganization, and fixes #9

Merged
merged 128 commits into from
Oct 4, 2024
Merged

Conversation

nabalone
Copy link
Collaborator

@nabalone nabalone commented Sep 30, 2024

This change is Reviewable

@nabalone
Copy link
Collaborator Author

Before merging we will need to change the trigger branch to main in release.yml

@nabalone nabalone marked this pull request as draft September 30, 2024 23:38
Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

Reviewed 76 of 76 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nabalone)


package.json line 35 at r1 (raw file):

  },
  "dependencies": {
    "@storybook/test-runner": "^0.19.1"

I'm guessing this is needed so we can publish to git?
It seems strange to have part of storybook as part of dependencies.


components/language-chooser/common/find-language/vitest.config.ts line 9 at r1 (raw file):

  //     requireAssertions: true,
  //   },
  // },

I thought we had decided we wanted this check.

Copy link
Collaborator Author

@nabalone nabalone left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @andrew-polk)


package.json line 35 at r1 (raw file):

Previously, andrew-polk wrote…

I'm guessing this is needed so we can publish to git?
It seems strange to have part of storybook as part of dependencies.

Moved it to dev dependencies


components/language-chooser/common/find-language/vitest.config.ts line 9 at r1 (raw file):

Previously, andrew-polk wrote…

I thought we had decided we wanted this check.

the requireAssertions check is introduced in vitest 2 and I downgraded to vitest 1, probably for compatibility. Not sure what to do now

Copy link
Collaborator Author

@nabalone nabalone left a comment

Choose a reason for hiding this comment

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

Reviewable status: 73 of 76 files reviewed, 2 unresolved discussions (waiting on @andrew-polk)


components/language-chooser/common/find-language/vitest.config.ts line 9 at r1 (raw file):

Previously, nabalone (Noel) wrote…

the requireAssertions check is introduced in vitest 2 and I downgraded to vitest 1, probably for compatibility. Not sure what to do now

I will reevaluate whether we can use vitest 2

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nabalone)


.prettierrc line 2 at r2 (raw file):

{
  "singleQuote": false

When this got changed to true, I saw a bunch of things got changed to use single quotes. But now it with it going back to double, I don't see the corresponding changes being reverted.
Probably if we save one of those files now, we will get spurious changes?

I'm not sure what is worth doing about it now...
Maybe worth a quick chat about why this got switched back and forth?


components/language-chooser/common/find-language/vitest.config.ts line 9 at r1 (raw file):

I will reevaluate whether we can use vitest 2

That's worth a short experiment.
Else, just comment that we want this check but currently we have vitest1 which doesn't support it.

Copy link
Collaborator Author

@nabalone nabalone left a comment

Choose a reason for hiding this comment

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

Reviewable status: 65 of 76 files reviewed, 2 unresolved discussions (waiting on @andrew-polk)


components/language-chooser/common/find-language/vitest.config.ts line 9 at r1 (raw file):

Previously, andrew-polk wrote…

I will reevaluate whether we can use vitest 2

That's worth a short experiment.
Else, just comment that we want this check but currently we have vitest1 which doesn't support it.

Added comment


.prettierrc line 2 at r2 (raw file):

Previously, andrew-polk wrote…

When this got changed to true, I saw a bunch of things got changed to use single quotes. But now it with it going back to double, I don't see the corresponding changes being reverted.
Probably if we save one of those files now, we will get spurious changes?

I'm not sure what is worth doing about it now...
Maybe worth a quick chat about why this got switched back and forth?

Should be fixed now

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

Reviewed 12 of 12 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nabalone)

@nabalone
Copy link
Collaborator Author

nabalone commented Oct 4, 2024

I'm guessing we want to squash and merge this, and then I may need to manually move tags to allow the github actions to detect version and publish properly

@nabalone nabalone marked this pull request as ready for review October 4, 2024 19:14
Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

Looks ready to me. Yes, squash looks best.
I'll let you handle the merge, etc.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nabalone)

@nabalone nabalone merged commit fbe41f5 into main Oct 4, 2024
1 check passed
@nabalone nabalone deleted the publishing_config branch October 4, 2024 19: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.

2 participants