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

Temporarily freeze react version, upgrade other dependencies #7227

Merged
merged 3 commits into from
May 14, 2024

Conversation

jcfilben
Copy link
Collaborator

What does this PR do?

Temporarily freeze react version at v18.2.0 until we have a chance to do that upgrade (tracked here: #7127).

This PR upgrades some other dependencies

Where should the reviewer start?

What testing has been done on this PR?

How should this be manually tested?

Do Jest tests follow these best practices?

  • screen is used for querying.
  • The correct query is used. (Refer to this list of queries)
  • asFragment() is used for snapshot testing.

Any background context you want to provide?

What are the relevant issues?

Screenshots (if appropriate)

Do the grommet docs need to be updated?

no

Should this PR be mentioned in the release notes?

no

Is this change backwards compatible or is it a breaking change?

backwards compatible

@jcfilben jcfilben requested review from britt6612 and taysea May 13, 2024 22:17
@@ -464,8 +464,8 @@ exports[`Anchor matches icon size to size prop when theme.icon.matchSize is true
-webkit-flex: 0 0 auto;
-ms-flex: 0 0 auto;
flex: 0 0 auto;
width: NaNpx;
height: NaNpx;
width: NaNxsmall;
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want these? I remember they are from grommet-icons wasn't sure if we looked into why they changed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm guessing it is coming from this PR, but not positive: https://github.com/grommet/grommet-icons/pull/325/files
Either way I think visually this test wouldn't change. The width is just being replaced with another invalid option.
At some point we should look at the useSizedIcon function in Grommet, I think this is where the issue may be coming from. I think the icons are supposed to scale to the size of the Anchor. This isn't working currently and this PR doesn't change that it just changes the incorrect value to another incorrect value.
Screen Shot 2024-05-14 at 9 00 58 AM

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have a ticket somewhere to capture?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't look like it, I'll go ahead and make an issue

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Issue created: #7231

@jcfilben jcfilben merged commit dc6b63c into grommet:master May 14, 2024
14 checks passed
@jcfilben jcfilben deleted the freeze-react-version branch May 14, 2024 22:17
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.

None yet

3 participants