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.

Sign up for GitHub

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

Jump to bottom

fix: icons, interactionsTests, testIds types exports #1369

Merged
merged 7 commits into from
Jun 28, 2023
Merged

fix: icons, interactionsTests, testIds types exports #1369

merged 7 commits into from
Jun 28, 2023

Conversation

SergeyRoyt
Copy link
Contributor

@SergeyRoyt SergeyRoyt requested a review from a team June 25, 2023 10:06
@SergeyRoyt SergeyRoyt self-assigned this Jun 25, 2023
shlomitc
shlomitc reviewed Jun 25, 2023
View reviewed changes
webpack/published-ts-components.js
@@ -103,8 +103,6 @@ const publishedTSComponents = {
useActiveDescendantListFocus: "hooks/useActiveDescendantListFocus",
useListenFocusTriggers: "hooks/useListenFocusTriggers",
useSwitch: "hooks/useSwitch",
// Don't remove next line
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty sure
We have this marker on the line 87

shlomitc
shlomitc reviewed Jun 25, 2023
View reviewed changes
package.json
Comment on lines +28 to +33
"interactionsTests": [
"./dist/esm/tests/interactions-utils.d.ts"
],
"testIds": [
"./dist/esm/tests/test-ids-utils.d.ts"
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these two needed to be publically exposed APIs?
The interactionsTests are mostly wrappers from regular DOM functions and they are very platform specific

Copy link
Contributor Author

Choose a reason for hiding this comment

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

testIds are containing ComponentDefaultTestId which contains all default data-testid for components and it's being used already in the monolith in cypress tests selectors
image

interactionsTests are exporting functions which helps to build storybook interaction tests and this endpoint is being used in the monday-ui-components rn
image

Also third one should be storybookComponents to export components we are using to build our storybook, but as they are not converted to ts, they are not here

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. I'm not too fond of it as a public API. it creates a coupling between Vibe's internal testing tools with others who consume it, making it harder to refactor. if that's the current state, then fine, but let's try to reduce such dependencies

@github-actions
Copy link
Contributor

A new prerelease version of this PR has been published: 2.10.3-types-exports-416097a90d.0

// To install this prerelease version using npm, please run the following command in your terminal:
npm i [email protected]
// If you prefer using Yarn, you can use the following command instead:
yarn add [email protected]

@SergeyRoyt SergeyRoyt requested a review from shlomitc June 25, 2023 11:31
@SergeyRoyt SergeyRoyt merged commit 7078fa3 into master Jun 28, 2023
@SergeyRoyt SergeyRoyt deleted the fix/sergeyro/types-exports branch June 28, 2023 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shlomitc shlomitc shlomitc approved these changes

Assignees

@SergeyRoyt SergeyRoyt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SergeyRoyt @shlomitc