Skip to content

[react-native] Add id prop to Text, TouchableWithoutFeedback and View components#62019

Closed
gabrieldonadel wants to merge 1 commit intoDefinitelyTyped:masterfrom
gabrieldonadel:feat/add-id-to-view
Closed

[react-native] Add id prop to Text, TouchableWithoutFeedback and View components#62019
gabrieldonadel wants to merge 1 commit intoDefinitelyTyped:masterfrom
gabrieldonadel:feat/add-id-to-view

Conversation

@gabrieldonadel
Copy link
Copy Markdown
Contributor

Please fill in this template.

This PR is the follow-up of facebook/react-native#34522, and add types for the new id prop to the Image, Text, TouchableWithoutFeedback and View components. It also adds new tests for the id prop.

@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Aug 29, 2022

@gabrieldonadel Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes of this PR in the Playground.

Status

  • ✅ No merge conflicts
  • ✅ Continuous integration tests have passed
  • 🕐 Most recent commit is approved by a DT maintainer

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 62019,
  "author": "gabrieldonadel",
  "headCommitOid": "098db8493a033e29df6b99485c35f73f1e23d81c",
  "mergeBaseOid": "2c4c1d0b106a1305c384cde84bb14bbf1a938d2c",
  "lastPushDate": "2022-08-30T01:12:07.000Z",
  "lastActivityDate": "2022-09-19T19:54:19.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "react-native",
      "kind": "edit",
      "files": [
        {
          "path": "types/react-native/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react-native/test/index.tsx",
          "kind": "test"
        }
      ],
      "owners": [
        "alloy",
        "huhuanming",
        "iRoachie",
        "timwangdev",
        "kamal",
        "alexdunne",
        "swissmanu",
        "bm-software",
        "mvdam",
        "esemesek",
        "mrnickel",
        "souvik-ghosh",
        "nossbigg",
        "saranshkataria",
        "tykus160",
        "jakebloom",
        "ceyhun",
        "mcmar",
        "theohdv",
        "romain-faust",
        "bebebebebe",
        "Naturalclar",
        "chinesedfan",
        "vtolochk",
        "SychevSP",
        "RageBill",
        "sasurau4",
        "256hz",
        "doumart",
        "drmas",
        "jeremybarbet",
        "ds300",
        "natsathorn",
        "connectdotz",
        "alexeymolchan",
        "alexbrazier",
        "kuasha420",
        "phvillegas",
        "eps1lon",
        "ZihanChen-MSFT",
        "kelset",
        "MateWW"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "changereq",
      "reviewer": "eps1lon",
      "date": "2022-09-19T19:54:19.000Z"
    },
    {
      "type": "approved",
      "reviewer": "kuasha420",
      "date": "2022-09-19T14:42:43.000Z",
      "isMaintainer": false
    },
    {
      "type": "approved",
      "reviewer": "iRoachie",
      "date": "2022-09-19T13:02:15.000Z",
      "isMaintainer": false
    }
  ],
  "mainBotCommentID": 1230961227,
  "ciResult": "pass"
}

@typescript-bot
Copy link
Copy Markdown
Contributor

@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Aug 29, 2022
@typescript-bot
Copy link
Copy Markdown
Contributor

@gabrieldonadel The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

@typescript-bot typescript-bot removed the The CI failed When GH Actions fails label Aug 30, 2022
@eps1lon
Copy link
Copy Markdown
Collaborator

eps1lon commented Aug 31, 2022

Am I correct in assuming this is a PR for an unreleased feature? If that's a case, could you convert this to a draft so that it doesn't create more notifications considering we can't merge it yet?

@gabrieldonadel
Copy link
Copy Markdown
Contributor Author

Am I correct in assuming this is a PR for an unreleased feature? If that's a case, could you convert this to a draft so that it doesn't create more notifications considering we can't merge it yet?

Sure, this PR is related to facebook/react-native#34522, I'm converting this back to a draft as you suggested then

@gabrieldonadel gabrieldonadel marked this pull request as draft August 31, 2022 12:00
@gabrieldonadel gabrieldonadel marked this pull request as ready for review September 19, 2022 12:51
@typescript-bot typescript-bot added the Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. label Sep 19, 2022
@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Sep 19, 2022

@gabrieldonadel
Copy link
Copy Markdown
Contributor Author

@eps1lon facebook/react-native#34522 has been merged and 0.70 release, I belive this will be available in the next release

@typescript-bot typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Sep 19, 2022
Copy link
Copy Markdown
Contributor

@kuasha420 kuasha420 left a comment

Choose a reason for hiding this comment

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

LGTM

@lunaleaps
Copy link
Copy Markdown
Contributor

lunaleaps commented Sep 19, 2022

Thanks @gabrieldonadel for adding this! Now that we've copied these types over to react-native (https://github.com/facebook/react-native/blob/main/types/index.d.ts), I'll copy your changes over and from now on we can just submit PRs to the react-native repo

Copy link
Copy Markdown
Collaborator

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

The merge commit for the linked PR is not tagged with any release yet. It was also merged 10 days ago while 0.70 was released 14 days ago. 0.70.1 release notes make no mention of this change. reactnative.dev docs also do not mention this prop.

The types in this repository reflect the documented and released API. Not the head of main in facebook/react-native.

Until the linked change is release, this should not be merged. I suggest going back to draft and re-requesting review once it's released.

@typescript-bot typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed Owner Approved A listed owner of this package signed off on the pull request. Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. labels Sep 19, 2022
@typescript-bot
Copy link
Copy Markdown
Contributor

@gabrieldonadel One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Sep 20, 2022
Summary:
Changelog: [Internal] - Apply changes from recent `id` property adds. These are changes by
gabrieldonadel from DefinitelyTyped/DefinitelyTyped#62019

Reviewed By: cipolleschi

Differential Revision: D39646408

fbshipit-source-id: d75f2e2b5b3d157825c4aaf21775e0d5165383ee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Critical package Revision needed This PR needs code changes before it can be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants