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

Update react-contexify to 6.0.0 #5725

Merged
merged 5 commits into from
May 28, 2024
Merged

Update react-contexify to 6.0.0 #5725

merged 5 commits into from
May 28, 2024

Conversation

benwolfram
Copy link
Collaborator

@benwolfram benwolfram commented May 22, 2024

Problem:
As apart of modernizing and updating context menus, bumping react-contexify to the latest version following their migration guide.

Fix:'

This PR fixes keyboard navigation for the context menu, however, events are still bubbling up to the editor. Will look at this next.

  • Bumping to version 6.0.0

  • Changing how show is called, with the event

  • Updating classes and redoing the styles

  • Removing explicit 28px heights

  • Using onVisibilityChange

  • Typescript type fixes

  • Removing the existing patch?

    • I am not sure what this is even, but I realized it was tied to 5.0.0 version, so it seemed irrelevant now. I assume there is a good reason why it exists, so please let me know if this needs to be redone for v6 and how that is done

Manual Tests:
I hereby swear that:

  • I opened a hydrogen project and it loaded
  • I could navigate to various routes in Preview mode

Relates to #5643

@benwolfram benwolfram self-assigned this May 22, 2024
@@ -444,7 +444,6 @@
"eslint-plugin-jsx-a11y@6.5.1": "patches/eslint-plugin-jsx-a11y@6.5.1.patch",
"karma-mocha-reporter@2.2.5": "patches/karma-mocha-reporter@2.2.5.patch",
"keycode@2.2.1": "patches/keycode@2.2.1.patch",
"react-contexify@5.0.0": "patches/react-contexify@5.0.0.patch",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As noted in PR description, let me know if (and how) this needs to be re-created for v6.

Copy link
Contributor

github-actions bot commented May 22, 2024

Try me

Copy link

relativeci bot commented May 22, 2024

#12700 Bundle Size — 62.24MiB (~-0.01%).

b0a37dc(current) vs 6339ece master#12690(baseline)

Warning

Bundle contains 51 duplicate packages – View duplicate packages

Bundle metrics  Change 3 changes Improvement 1 improvement
                 Current
#12700
     Baseline
#12690
Improvement  Initial JS 45.31MiB(~-0.01%) 45.32MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 26.2% 22.06%
No change  Chunks 30 30
No change  Assets 33 33
No change  Modules 4293 4293
No change  Duplicate Modules 521 521
Change  Duplicate Code 31.79%(+0.03%) 31.78%
No change  Packages 448 448
No change  Duplicate Packages 51 51
Bundle size by type  Change 2 changes Improvement 2 improvements
                 Current
#12700
     Baseline
#12690
Improvement  JS 62.23MiB (~-0.01%) 62.23MiB
Improvement  HTML 11.16KiB (-0.32%) 11.2KiB

Bundle analysis reportBranch context-menu-updateProject dashboard

Copy link
Contributor

github-actions bot commented May 22, 2024

Performance test results:
(Chart1)
(Chart2)

@benwolfram
Copy link
Collaborator Author

benwolfram commented May 22, 2024

Would love some assistance on:

  • What are / is the difference between Test Editor Shard 1 vs. 2
    • I think # 1 is equivalent to pnpm run test so I am able to run that locally and debug (this is just failing the flaky/unrelated test)
    • How I can run/debug the # 2 test suite locally
  • Getting performance tests fixed

@benwolfram benwolfram marked this pull request as ready for review May 22, 2024 17:43
@benwolfram benwolfram requested a review from a team May 22, 2024 17:43
@benwolfram
Copy link
Collaborator Author

The "try me" version doesn't seem to be working at all 😕, but it definitely does locally for me.

show({
event: event,
position:
localXHack_KILLME === 'local-x-coord-KILLME'
Copy link
Contributor

Choose a reason for hiding this comment

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

@benwolfram are we tracking this? we should really kill this localXHack!
this was meant as a temporary fix in a demo deadline push a few weeks ago, simply put here because we only had five minutes to fix the offset.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, this wasn't on my todo list. I am not even sure what it is tbh, but I can take a look here soon in a future PR, as I was intending to wrap work on Context Menu.

package-lock.json Outdated Show resolved Hide resolved
Copy link
Contributor

@balazsbajorics balazsbajorics left a comment

Choose a reason for hiding this comment

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

left some minor feedback but mostly looks good!

@balazsbajorics
Copy link
Contributor

it looks like this PR broke The navigator component picker context menu Karma tests

@benwolfram benwolfram merged commit 4d3a45a into master May 28, 2024
13 checks passed
@benwolfram benwolfram deleted the context-menu-update branch May 28, 2024 17:12
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