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

Add fetchPriority to <img> and <link> #25927

Merged
merged 2 commits into from Dec 23, 2022

Conversation

styfle
Copy link
Contributor

@styfle styfle commented Dec 22, 2022

Summary

How did you test this change?

I tried this but it didn't work

yarn build --type=UMD_DEV react/index,react-dom && cd fixtures/attribute-behavior && yarn install && yarn start

@sebmarkbage sebmarkbage requested a review from eps1lon Dec 22, 2022
@styfle styfle marked this pull request as ready for review Dec 22, 2022
Copy link
Collaborator

@eps1lon eps1lon left a comment

I tried this but it didn't work

Thank you for reporting! I filed #25928 and will update the fixture with the correct values.

fixtures/attribute-behavior/src/attributes.js Outdated Show resolved Hide resolved
1. always read correct property (to test compat with lowercase prop name)
1. use a known string value  to test read/write parity

Ran fixture with meta tags enabled in Chrome Version 107.0.5304.110 (Official Build) (64-bit) on Ubuntu 22.04
@eps1lon
Copy link
Collaborator

eps1lon commented Dec 23, 2022

@styfle Could you check if the value in the attribute fixture match your expectation?

@@ -573,6 +573,24 @@ const attributes = [
tagName: 'path',
read: getSVGAttribute('externalResourcesRequired'),
},
{
name: 'fetchPriority',
overrideStringValue: 'high',
Copy link
Contributor Author

@styfle styfle Dec 23, 2022

Choose a reason for hiding this comment

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

@eps1lon Why would this override? I'm not sure I understand the purpose of overrideStringValue 🤔

Copy link
Collaborator

@eps1lon eps1lon Dec 23, 2022

Choose a reason for hiding this comment

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

Just so that we test with a valid value instead of an arbitrary one that you'd never use in a real application

@sebmarkbage sebmarkbage merged commit de7d1c9 into facebook:main Dec 23, 2022
36 of 37 checks passed
@styfle styfle deleted the add-fetch-priority branch Dec 23, 2022
github-actions bot pushed a commit that referenced this pull request Dec 23, 2022
<!--
  Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.

Before submitting a pull request, please make sure the following is
done:

1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
  2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn debug-test --watch TestName`, open
`chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

- Fixes #25682

## How did you test this change?

I tried this but it didn't work
```
yarn build --type=UMD_DEV react/index,react-dom && cd fixtures/attribute-behavior && yarn install && yarn start
```

Co-authored-by: eps1lon <silbermann.sebastian@gmail.com>

DiffTrain build for [de7d1c9](de7d1c9)
[View git log for this commit](https://github.com/facebook/react/commits/de7d1c90718ea8f4844a2219991f7115ef2bd2c5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: <img fetchPriority> attribute is not supported
4 participants