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

Javascript error when editing shadow after resetting styles #65930

Closed
2 tasks done
benniledl opened this issue Oct 8, 2024 · 8 comments · Fixed by #65935
Closed
2 tasks done

Javascript error when editing shadow after resetting styles #65930

benniledl opened this issue Oct 8, 2024 · 8 comments · Fixed by #65935
Assignees
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@benniledl
Copy link
Contributor

benniledl commented Oct 8, 2024

Description

Similar issue as #65774 exists for Shadows

For details visit #65774

Step-by-step reproduction instructions

  1. Go to global styles
  2. Shadows
  3. add an inidvidual shadow
  4. click on the individual shadow
  5. reset styles
  6. edit currently selected shadow

Screenshots, screen recording, code snippet

Blog.Home.Template.Testsite.Editor.WordPress.Mozilla.Firefox.2024-10-08.08-29-32.mp4

Environment info

No response

Please confirm that you have searched existing issues in the repo.

  • Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

  • Yes
@benniledl benniledl added the [Type] Bug An existing feature does not function as intended label Oct 8, 2024
@t-hamano t-hamano added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Oct 8, 2024
@nith53
Copy link

nith53 commented Oct 8, 2024

I am also able to replicate this issue.
Image

@ciampo
Copy link
Contributor

ciampo commented Oct 8, 2024

My instinct tells me that this is not the same issue as #65774, since it's not triggered by leaving the shadows screen. I'll timebox an effort and see if I can come up with a fix

I believe the issue is that, when resetting styles, the current shadow entry gets removed. And therefore, and subsequent edit can't be applied (and triggers an error), because it's trying to edit a shadow that doesn't exist anymore in the settings. We should probably just navigate back to the parent screen when resetting styles.

@ciampo
Copy link
Contributor

ciampo commented Oct 8, 2024

@benniledl @nith53 can you test if #65935 fixes it? It's a draft for now, but I'm going to add a proper description/screencast in a bit

@benniledl
Copy link
Contributor Author

@ciampo sorry I'm not sure how I can test a gutenberg PR,
How I would test PR's would be do just modify the files via FTP according to the PR. But I think the javascript files of gutenberg are minified in wordpress so I don't know how to modify the gutenberg source and then "build" the files. Could you share a link on how to test?

@t-hamano
Copy link
Contributor

t-hamano commented Oct 8, 2024

I believe the issue is that, when resetting styles, the current shadow entry gets removed. And therefore, and subsequent edit can't be applied (and triggers an error), because it's trying to edit a shadow that doesn't exist anymore in the settings. We should probably just navigate back to the parent screen when resetting styles.

I think this is correct.

The font size preset implementation relies entirely on the useGlobalSetting() hook. Therefore, if we reset the global styles in a custom font size preset panel, a critical error occurs because it references a preset that does not exist.

On the other hand, the shadow panel logic is a little different, and it keeps the preset obtained via the useGlobalSetting() hook in the state. So even if we reset the global styles in a custom shadow preset panel, the preset still exists in the state, so no critical error occurs. But of course this is not the intended behavior.

Shadow presets may need to be refactored in the future to not depend on the state, but the approach attempted in #65935 is similar to that of font size presets and I think it makes sense.

@ciampo
Copy link
Contributor

ciampo commented Oct 8, 2024

sorry I'm not sure how I can test a gutenberg PR, How I would test PR's would be do just modify the files via FTP according to the PR. But I think the javascript files of gutenberg are minified in wordpress so I don't know how to modify the gutenberg source and then "build" the files. Could you share a link on how to test?

The easiest way to test the changes is to visit https://playground.wordpress.net/gutenberg.html and input the PR number (ie. 65935).

If you want to run a local version of the code on your machine, you can follow this documentation.

@ciampo
Copy link
Contributor

ciampo commented Oct 8, 2024

Shadow presets may need to be refactored in the future to not depend on the state, but the approach attempted in #65935 is similar to that of font size presets and I think it makes sense.

Agreed, aki!

@nith53
Copy link

nith53 commented Oct 8, 2024

@benniledl @nith53 can you test if #65935 fixes it? It's a draft for now, but I'm going to add a proper description/screencast in a bit

Tested via the Playground. The issue seems to be resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants