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

Convert FocalPointPicker tests to TypeScript #61373

Merged
merged 6 commits into from
May 10, 2024

Conversation

jpstevens
Copy link
Contributor

What?

This PR converts the index and media tests for the FocalPointPicker to TypeScript.

Why?

Adding TypeScript support resolves this issue: #60529

How?

  • converted both files to .tsx
  • fixed missing property errors by passing default { ...props } to components (similar to this test)
  • for "should handle legacy string values" test, cast string values as unknown as number to avoid changing FocalPoint type to officially support string values as well as number

If the preference is to officially support string values for focal points let me know and I'll update FocalPoint type to Record< FocalPointAxis, number | string > - however this will require further changes to the FocalPointPicker implementation.

Testing Instructions

npm run test:unit -- packages/components/src/focal-point-picker tests pass (see screenshot below)
npm run build:package-types returns a zero exit code

Screenshots or screencast

image
@jpstevens jpstevens requested a review from ajitbohra as a code owner May 3, 2024 20:37
Copy link

github-actions bot commented May 3, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: jpstevens <jpstevens@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link

github-actions bot commented May 3, 2024

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @jpstevens! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label May 3, 2024
@mirka mirka requested a review from a team May 7, 2024 14:00
@mirka mirka added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Package] Components /packages/components labels May 7, 2024
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Thanks for working on it 👍

I think this looks good, I've only offered a couple of minor suggestions.


const props: FocalPointPickerMediaProps = {
alt: '',
src: '',
Copy link
Member

Choose a reason for hiding this comment

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

Literally every test will have the data-testid="media" prop, should we add this to props and remove that prop from the tests?

We might need to add the data-testid prop to FocalPointPickerMediaProps since that prop doesn't exist there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question - I've updated the default prop object to include 'data-testid': 'media'

The change is only required for testing, so I opted not to change the underlying FocalPointPickerMediaProps type definition.

The current implementation of Media uses pass-through props, which allow any prop to be passed to the child component, including data-testid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly, I couldn't find any tests in the codebase where the data-testid prop is not repeated on each component:

  • Elevation repeats data-testid="elevation"
  • Grid repeats data-testid="grid"
  • Panel repeats data-testid="inner-content"

There is one example that extracts the data-testid value to a const, but it doesn't prevent the prop repetition on the components:

  • Icon repeats data-testid={ testId }
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for taking such a thorough look! This was a very minor detail and TBH I'm good with you've come up with 👍

packages/components/src/focal-point-picker/test/index.tsx Outdated Show resolved Hide resolved
jpstevens and others added 2 commits May 9, 2024 14:03
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Looks good. Happy to ship this for you, but could I ask you to rebase it to the latest trunk before that? Maybe that will solve the compressed size check failure.


const props: FocalPointPickerMediaProps = {
alt: '',
src: '',
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for taking such a thorough look! This was a very minor detail and TBH I'm good with you've come up with 👍

@jpstevens
Copy link
Contributor Author

I synced with trunk, seems to have resolved the failing task. Thanks for the review @tyxla!

@tyxla tyxla merged commit d489fb3 into WordPress:trunk May 10, 2024
60 checks passed
@github-actions github-actions bot added this to the Gutenberg 18.4 milestone May 10, 2024
@jpstevens jpstevens deleted the update/type-focal-point-picker-tests branch May 10, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Package] Components /packages/components [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
3 participants