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

Do not SSR in legacy themes #38

Merged
merged 3 commits into from
Jun 27, 2024
Merged

Do not SSR in legacy themes #38

merged 3 commits into from
Jun 27, 2024

Conversation

nateconley
Copy link
Collaborator

Description of the Change

Disallows legacy themes from server side render in the block editor. These older themes do not have the full stylesheet enabled in the block editor and will instead apply core's forms.css for admin forms.

Closes #2

How to test the Change

  • Form is rendered in the block editor for Twenty Twenty-Four when not focuses.
  • Form does not render in the block editor for Twenty Twenty-One

Changelog Entry

Enhances: new block

Credits

@nateconley

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@nateconley nateconley requested a review from dkotter June 27, 2024 04:01
@nateconley nateconley self-assigned this Jun 27, 2024
@github-actions github-actions bot added this to the 1.6.0 milestone Jun 27, 2024
@github-actions github-actions bot added the needs:code-review This requires code review. label Jun 27, 2024
@nateconley nateconley mentioned this pull request Jun 27, 2024
2 tasks
Copy link
Collaborator

@dkotter dkotter left a comment

Choose a reason for hiding this comment

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

Code here looks fine to me, though it would be great if there was a better way to detect when to turn off SSR instead of just using a hardcoded list.

I guess also curious if we want to put effort into adjusting any front-end styling? In testing with Twenty Twenty One, styling looked pretty good (I'm assuming because of the custom styling that theme uses). But in testing with Twenty Twenty Four, there's definitely some improvements we could make, like better padding for the form inputs and better button styling:

Form styling in the Twenty Twenty Four theme

But I also realize most sites will have custom styling for form inputs, so always a balance of not wanting to impact those styles while still having a good default baseline.

@nateconley
Copy link
Collaborator Author

@dkotter To address your concerns on Twenty Twenty-Four, here is what Contact Form 7 looks like:
Screenshot 2024-06-27 at 6 29 13 AM

I think the best path forward here might be to add field "Padding" and "Button" settings to the "Custom Styling" section - I can add that in.

For the denylist, I think this is going to be difficult to accomplish consistently, as we really don't have a reliable way to figure out how much of the theme style is added to the editor.

@dkotter
Copy link
Collaborator

dkotter commented Jun 27, 2024

@nateconley Sorry, just saw your message as I was working on this. I've added in some basic padding to all form elements and things look much better to me now with the ability to still easily override that on a theme level:

Adjusted form styles

Though feel free to adjust further if needed

@nateconley
Copy link
Collaborator Author

@dkotter If this is working well across themes, I think this is a great path forward. Thanks!

@nateconley
Copy link
Collaborator Author

@dkotter I gave this a quick look in themes 2024, 2023, 2022, and 2021 and it looks great!

@dkotter dkotter merged commit 3eb0fd6 into develop Jun 27, 2024
4 checks passed
@dkotter dkotter deleted the feat/theme-compat branch June 27, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add theme compatibility
2 participants