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

fix(core): reference links not opening correct pane #5116

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

rexxars
Copy link
Member

@rexxars rexxars commented Nov 2, 2023

Description

Fixes #5105

#5093 introduced a regression where references can no longer be opened in-place.
This is caused by removing the documentId from being passed to the state link.

This revert this change, but reintroduces warnings about passing it to DOM elements, so it is not a perfect solution. I struggled to figure out what is actually being passed to what here - a lot of as/data-as/Card/StyledCard magic and layers of abstraction, so I would appreciate if either of you had time to fix this properly and push to this branch.

This warrants a hotfix release once addressed.

What to review

  • That it fixes the problem
  • What the proper solution is

Notes for release

  • Fixed an issue where references would not open in-place, instead yielding an error about being changed after opening

Copy link

vercel bot commented Nov 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
performance-studio ✅ Ready (Inspect) Visit Preview Nov 2, 2023 10:38pm
test-studio ✅ Ready (Inspect) Visit Preview Nov 2, 2023 10:38pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Nov 2, 2023 10:38pm

Copy link
Contributor

github-actions bot commented Nov 2, 2023

No changes to documentation

Copy link
Contributor

github-actions bot commented Nov 2, 2023

Component Testing Report Updated Nov 2, 2023 10:45 PM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 20s 12 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 4s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 10s 3 0 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 9s 6 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 26s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 13s 9 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 51s 18 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 10s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 5s 3 0 0

@rexxars rexxars added this pull request to the merge queue Nov 3, 2023
Merged via the queue into next with commit ba7a558 Nov 3, 2023
37 checks passed
@rexxars rexxars deleted the fix/reference-link branch November 3, 2023 00:08
@robinpyon
Copy link
Contributor

robinpyon commented Nov 3, 2023

Apologies – I should have been much more stringent here, especially given the presence of the polymorphic as prop.

I did have a quick look into this and I believe the following workaround removes the error, though would appreciate your eyes @bjoerge

// ReferenceLinkCard.tsx

// Before
const as = documentType ? asProp : 'div'

// After
const as = documentId || documentType ? asProp : 'div'

return (
  <StyledCard
    ...
    forwardedAs={as}
    ...
  />
)

What's happening:

  • ReferenceLinkCard renders multiple times and receives both documentId and documentType, but not both to start. It renders multiple times, and receives documentId first (documentType will be undefined)
  • During this render pass where only documentId is valid, as will still be set to div, yielding the errors about documentId not being a valid DOM attribute
  • At a later render where documentType is valid, as will then be set to asProp, which ends up being ReferenceChildLink (once you follow it up the provider chain)
  • At this point, ReferenceChildLink renders, using documentId and documentType props (and doesn't pass these to the underlying DOM element)
  • I believe this explains why we don't receive errors for documentType

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.

References Forced to Reload When Opened Under Parent
3 participants