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.

Sign up for GitHub

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

Jump to bottom

[BBT-76] Schema UI - Border Component #18

Merged
merged 18 commits into from
Aug 18, 2023
Merged

[BBT-76] Schema UI - Border Component #18

merged 18 commits into from
Aug 18, 2023

Conversation

scottblackburn
Copy link
Collaborator

@scottblackburn scottblackburn commented Aug 1, 2023 •
edited
Loading

Description

This pull request provides a base for creating UI components that use the living schema file as a source of truth.

This PR provides some utils that allow the schema file to be parsed and the properties for each item (border, shadow, spacing etc) are mapped to components.

The border component is the only component in the scope of this PR and can be used as a guide other UI elements.

The example is tested using this example style: <Border selector="styles.blocks.core/pullquote.border" />

Change Log

  • Adds schema parser
  • Adds component map so that UI name can match the property name in the schema
  • Adds border component UI
  • Provides an example using the pull quote border

Steps to test

  • Sign into admin
  • Appearance > styles editor
  • Change border settings
  • Save
  • Create new page
  • Add new pull quote block
  • Border should reflect the saved settings

Screenshots/Videos

Test video: http://bigbite.im/v/FCOHyl

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

g-elwell
g-elwell requested changes Aug 3, 2023
View reviewed changes
Copy link
Member

@g-elwell g-elwell left a comment

Choose a reason for hiding this comment

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

I think we might be missing a piece of the puzzle by not mapping over the schema file to render out our fields, and looking through this code I worry implementing this would conflict with the way we're mapping things here, so it feels like something we should address sooner rather than later.

We were previously doing something similar with the theme.json where we would loop over each property and - depending on the value - render a component or recursively continue looping through the file.

I feel we should be doing something similar using the schema file, maybe via the top level properties object?

image

For example we might render e.g. a <Border> component when we hit a border attribute, but also to keep track of the our current position in relation to the mapping, so for example instead of rendering:

<Border selector="styles.blocks.core/pullquote.border" />

we could render:

<Border selector={currentPath} />

which would look for a matching value from theme.json in the same cursor position. Again, we did something similar when mapping the theme.json file so may be able to carry over aspects of that approach, but I realise it isn't as straightforward as that!

@scottblackburn
Copy link
Collaborator Author

@g-elwell this makes sense, we just didn't add to this PR but @chrishbite is looking at this now. I don't think it needs to block this PR as we can get going with the other components and just make the selectors dynamic, it might be more effective to do this once the components are in place.

@g-elwell g-elwell self-requested a review August 7, 2023 09:07
@chrishbite
Copy link
Contributor

chrishbite commented Aug 7, 2023 •
edited
Loading

Hey @g-elwell @scottblackburn thanks for taking a look at this, that makes sense to me;

themer/src/editor/components/fields/ThemerComponent.js

Line 154 in 6c9f40a

<Border selector="styles.blocks.core/pullquote.border" />

So this was hardcoded just to demo the component working with a random block which supported the border attribute, with the goal that these components would eventually be used dynamically with a generated selector from the schema parser when a border setting was encountered at any nested level 👍

@g-elwell
Copy link
Member

g-elwell commented Aug 7, 2023

Hey @g-elwell @scottblackburn thanks for taking a look at this, that makes sense to me;

themer/src/editor/components/fields/ThemerComponent.js

Line 154 in 6c9f40a

<Border selector="styles.blocks.core/pullquote.border" />

So this was hardcoded just to demo the component working with a random block which supported the border attribute, with the goal that these components would eventually be used dynamically with a generated selector from the schema parser when a border setting was encountered at any nested level 👍

Yeah, I remember some conversations about this so sorry if I seem to be contradicting what was said previously! My concern was around whether a method of mapping through the file to render components would be compatible with the approaches here. Sounds like both of you and @scottblackburn are happy this isn't the case and that's good enough for me.

@chrishbite
Copy link
Contributor

chrishbite commented Aug 7, 2023 •
edited
Loading

Yeah, I remember some conversations about this so sorry if I seem to be contradicting what was said previously! My concern was around whether a method of mapping through the file to render components would be compatible with the approaches here. Sounds like both of you and @scottblackburn are happy this isn't the case and that's good enough for me.

Thanks for taking a look at this @g-elwell much appreciated, I can make adjustments to the approach if needed.

I'm going to start work on the schema renderer hopefully soon so with that, we should be able to confirm if this way is working as expected and would work dynamically, might be worth discussing this in the catchup as well?

g-elwell
g-elwell requested changes Aug 7, 2023
View reviewed changes
src/editor/components/fields/ThemerComponent.js Outdated
@@ -73,7 +85,7 @@ const ThemerComponent = () => {
* saves edited entity data
*/
const save = async () => {
dispatch( 'core' ).undo();
// dispatch( 'core' ).undo();
Copy link
Member

Choose a reason for hiding this comment

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

Is this no longer required? If so should we remove it

Copy link
Contributor

@chrishbite chrishbite Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I have removed this, I believe this was required originally when updating but with the new components, on a Save and page refresh the settings would revert back one edit in the history.

themer/src/editor/components/fields/ThemerComponent.js

Lines 87 to 89 in b2c96dc

const save = async () => {
try {
await dispatch( 'core' ).saveEditedEntityRecord(

src/editor/components/fields/ThemerComponent.js Outdated
@@ -98,6 +110,18 @@ const ThemerComponent = () => {
);
};

/* TODO: refactor */
Copy link
Member

Choose a reason for hiding this comment

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

does this need a refactor? can we add a note explaining why, or preferably refactor now OR remove the comment

Copy link
Contributor

@chrishbite chrishbite Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So this is related to initialising the component map and is not really related to the building of the components themselves, was needed only to get the demo working for a component.

This would need to be refactored and moved completely out of this location and incorporated into the loading state of the full application based around the fetching and parsing of the external schema file.

src/editor/components/fields/ThemerComponent.js Outdated
@@ -108,13 +132,30 @@ const ThemerComponent = () => {

return (
<>
<style>{ previewCss }</style>
Copy link
Member

Choose a reason for hiding this comment

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

this could cause styles to bleed into unexpected areas, since some of the CSS rules are scoped to body and designed to be used within a block editor instance.

I can see a small strip of background colour at the bottom of the page for example:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Good spot, I've ended up removing this as the issue I had originally doesn't seem to be reoccurring with it removed, colours in the Border component look to be working correctly.

themer/src/editor/components/fields/ThemerComponent.js

Lines 132 to 135 in ff19703

return (
<>
<div className="themer-topbar">
<Button isSecondary onClick={ () => reset() } text="Reset" />

src/editor/components/fields/ThemerComponent.js Outdated
Comment on lines 143 to 153
<EditorContext.Provider
value={ {
globalStylesId,
themeConfig,
} }
>
<StylesContext.Provider
value={ {
setUserConfig,
} }
>
Copy link
Member

Choose a reason for hiding this comment

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

Should we move the contexts to the outermost wrapper of this component?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've moved the context to the top of this component in 4076d63

src/editor/components/fields/ThemerComponent.js Outdated
</StylesContext.Provider>
</EditorContext.Provider>
{ /* demo */ }
{ /* <TabPanel
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should render the demo UI component inside the first tab, rather than comment out all of this area?

Copy link
Contributor

Choose a reason for hiding this comment

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

Restored the TabPanel and Fields, moved Border demo component into TabPanel in 4076d63

src/editor/ui/components/styles/Border.js Outdated
config = set( config, selector, borders );

setUserConfig( config );
}, [ borders ] );
Copy link
Member

Choose a reason for hiding this comment

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

I think that themeConfig, setUserConfig and selector should be in the dependency array here - https://react.dev/learn/removing-effect-dependencies#dependencies-should-match-the-code

Copy link
Contributor

@chrishbite chrishbite Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @g-elwell @scottblackburn I've refactored the Border component internals to avoid using useState so the useEffect is no longer needed and will resolve the above comment.

useState was originally used here to avoid a TOCTOU race condition, this seems to have corrected itself with the movement of the setUserConfig function to a context provider.

I also just spotted a bug with using useState internally, as the when the global Reset button is pressed although the editEntityRecord would be updated this would not be reflected on these individual components and would need a complete page refresh or React reinitialisation.

@chrishbite
Copy link
Contributor

chrishbite commented Aug 7, 2023 •
edited
Loading

Hey @g-elwell @scottblackburn I've left comments on all the points raised which should hopefully solve them all 👍

I'd recommend running through the test steps again now though please with numerous options as the Border component has been refactored, more details on this via #18 (comment)

@chrishbite
feat(blocks): implements the blocks tab
36169b9 
- border style component implemented for all supported blocks

- placeholder tab components created

- placeholder style components created

- removal of uneeded files and code

- file and folder restructure

- new search component
@chrishbite
refactor(blocks): use schema and theme.json for block data source
c4fb462
@chrishbite
Copy link
Contributor

chrishbite commented Aug 14, 2023 •
edited
Loading

@g-elwell @scottblackburn

This has been refactored to use the theme.json schema and theme.json styles properties as the source of truth for the Blocks list and will display the Border component on supported Blocks, includes various code changes and restructuring needed for displaying components dynamically.

Steps To Test

Border Component

  1. Open up theme.json in Twenty Twenty Three theme
  2. Add the below to any styles.blocks.BLOCK_NAME properties that support it
"border": {
  "width": "1px"
},
  1. Load Styles Editor
  2. View Blocks tab
  3. Expand the Blocks edited from above
  4. The Border control component should be showing with set settings
  5. Edit block Border settings via the component and save
  6. Refresh page, saved settings should be persistent
  7. Modify the Preview component to include your testing Blocks
  8. Border settings should be reflected in the preview

Block List

  1. Visit Styles Editor > Blocks and confirm core/list is not displaying (Must not already be in theme.json styles.blocks)
  2. Add the below into styles.blocks in theme.json
"core/list": {
  spacing": {
    "padding": {
      "left": "var(--wp--preset--spacing--30)",
      "right": "var(--wp--preset--spacing--30)"
     }
  }
},
  1. Refresh and visit Styles Editor > Blocks again and confirm core/list is now displaying in this list

Search

  1. Visit Styles Editor > Blocks
  2. Add a search string in the Search box
  3. Blocks list should reflect your search term

Change Log

  • Border style component implemented
  • Block list generated from theme.json schema and current theme.json
  • Placeholder tab components created
  • Placeholder style components created
  • Removal of unneeded files and code
  • File and folder restructure
  • New reusable search component

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@chrishbite chrishbite marked this pull request as draft August 14, 2023 12:43
@chrishbite chrishbite marked this pull request as ready for review August 15, 2023 10:42
@scottblackburn
Copy link
Collaborator Author

Adding test video: http://bigbite.im/v/qiLl1S

@scottblackburn scottblackburn merged commit c076ad6 into release/1.0.0 Aug 18, 2023
1 check passed
@g-elwell g-elwell mentioned this pull request Jul 26, 2024
Draft
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrishbite chrishbite chrishbite left review comments

@g-elwell g-elwell g-elwell approved these changes

@chrishall0 chrishall0 Awaiting requested review from chrishall0

@balw balw Awaiting requested review from balw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@scottblackburn @chrishbite @g-elwell