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

Add theme.json file and remove few theme support from php file #85

Merged
merged 3 commits into from
Dec 29, 2022

Conversation

devanshijoshi9
Copy link
Contributor

Description

  • Add theme.json file.

Checklist

  • Add theme.json file.
  • Remove some theme_supports from php file.

Fixes/Covers issue

Fixes #80

Note- Keep few of the theme support in php as I am unable to find theme support in theme.json file. Here is the reference.

@maitreyie-chavan maitreyie-chavan requested review from sagarjadhav and removed request for RahiDroid September 7, 2022 12:00
sagarjadhav
sagarjadhav previously approved these changes Sep 7, 2022
Copy link

@sagarjadhav sagarjadhav left a comment

Choose a reason for hiding this comment

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

Looks good!

@RahiDroid
Copy link
Member

RahiDroid commented Sep 7, 2022

Just a note, I believe hybrid themes require the CSS on FE to be added manually using the tokens generated by the theme.json file.

Please verify it once before letting it through.

@RahiDroid
Copy link
Member

Just a note, I believe hybrid themes require the CSS on FE to be added manually using the tokens generated by the theme.json file.

Please verify it once before letting it through.

@robicse11127 Please verify this, update the theme.json if there are any changes required, and test out the other changes in this PR.

@robicse11127
Copy link
Contributor

theme.json Looks okay to me

@RahiDroid
Copy link
Member

@robicse11127

Just a note, I believe hybrid themes require the CSS on FE to be added manually using the tokens generated by the theme.json file.

Please verify it once before letting it through.

Verified this or you're yet to do it?

@robicse11127
Copy link
Contributor

@RahiDroid
Adding styling in theme.json is applied in both editor and FE. So if any style added in the theme.json should also be impacted in the FE.

@RahiDroid
Copy link
Member

@robicse11127 The theme.json file misses out on demonstrating many things that are possible with it. For example, Color palettes, Typography related presets, Spacing presets, Default styling for blocks, Custom variables, etc.

Could you please include some examples demonstrating those in the theme.json file? The Blank theme is supposed to be design-neutral, so please include some basic palette and other presets for demonstration purpose only.

If you want, you can refer Twenty-TwentyThree theme's theme.json file.

@robicse11127
Copy link
Contributor

@RahiDroid Got it.
I have updated the theme.json with some example presets.

Copy link
Member

@RahiDroid RahiDroid left a comment

Choose a reason for hiding this comment

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

Some minor changes, almost there!

Copy link
Member

@RahiDroid RahiDroid left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @robicse11127 and @devanshijoshi9! 🚀

@RahiDroid RahiDroid merged commit 920bb6a into master Dec 29, 2022
@RahiDroid RahiDroid deleted the feat/add-theme-json-file branch December 29, 2022 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add theme.json file
4 participants