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

chore(ui-react): lint primitives (T - Z) #3297

Merged
merged 7 commits into from
Jan 12, 2023
Merged

Conversation

calebpollman
Copy link
Member

Description of changes

Lint primitives T* - Z*

Issue #, if available

NA

Description of how you validated changes

yarn react lint && yarn react test

Checklist

  • PR description included
  • yarn test passes
  • Tests are updated
  • No side effects or sideEffects field updated
  • Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@calebpollman calebpollman requested a review from a team as a code owner January 12, 2023 01:04
@changeset-bot
Copy link

changeset-bot bot commented Jan 12, 2023

🦋 Changeset detected

Latest commit: 423715e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@aws-amplify/ui-react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

0618
0618 previously approved these changes Jan 12, 2023
// 'X',
// 'Y',
// 'Z'
'T',
Copy link
Contributor

Choose a reason for hiding this comment

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

If all primitives are done, can we remove these and go back to src/primitives/**/*?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no going back where we've never been 😄

All jokes aside will update the .eslintrc.js in a follow up

Copy link
Member Author

Choose a reason for hiding this comment

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

Went ahead and updated the lint config to remove the need templateJoin

// at this point the child defined (not null or undefined)
// it is NOT a TabItem, so log a message
if (child) {
console.warn(
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you explained this, but why are we not logging a message anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was very easy to bypass the isTabsType predicate, so the warning here was also fragile

@calebpollman calebpollman temporarily deployed to ci January 12, 2023 01:28 — with GitHub Actions Inactive
@calebpollman calebpollman temporarily deployed to ci January 12, 2023 01:28 — with GitHub Actions Inactive
@calebpollman calebpollman temporarily deployed to ci January 12, 2023 01:28 — with GitHub Actions Inactive
joebuono
joebuono previously approved these changes Jan 12, 2023
@calebpollman calebpollman dismissed stale reviews from joebuono and 0618 via 62e67c8 January 12, 2023 17:36
@calebpollman calebpollman temporarily deployed to ci January 12, 2023 18:06 — with GitHub Actions Inactive
@calebpollman calebpollman temporarily deployed to ci January 12, 2023 18:06 — with GitHub Actions Inactive
@calebpollman calebpollman temporarily deployed to ci January 12, 2023 18:06 — with GitHub Actions Inactive
@calebpollman calebpollman temporarily deployed to ci January 12, 2023 18:06 — with GitHub Actions Inactive
wlee221
wlee221 previously approved these changes Jan 12, 2023
@calebpollman calebpollman temporarily deployed to ci January 12, 2023 19:00 — with GitHub Actions Inactive
@calebpollman calebpollman temporarily deployed to ci January 12, 2023 19:00 — with GitHub Actions Inactive
@calebpollman calebpollman temporarily deployed to ci January 12, 2023 19:00 — with GitHub Actions Inactive
@calebpollman calebpollman temporarily deployed to ci January 12, 2023 19:00 — with GitHub Actions Inactive
* Additionally, `value` is avaialble on the props of `TabItemPrimitive`, but is not present
* on `TabItemProps`. To mitigate this issue prefer usage of the props of `TabItemPrimitive`
`*/
type ExtendedTabItemProps = Parameters<typeof TabItemPrimitive>[0] & {
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL about Parameters util type!

Copy link
Contributor

@reesscot reesscot left a comment

Choose a reason for hiding this comment

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

LGTM

@calebpollman calebpollman merged commit 58b65b3 into main Jan 12, 2023
@calebpollman calebpollman deleted the ui-react/lint-primitives branch January 12, 2023 19:30
@github-actions github-actions bot mentioned this pull request Jan 12, 2023
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.

5 participants