-
Notifications
You must be signed in to change notification settings - Fork 0
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
[BBT-76] Schema UI - Border Component #18
Conversation
There was a problem hiding this 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?
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!
@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. |
Hey @g-elwell @scottblackburn thanks for taking a look at this, that makes sense to me;
So this was hardcoded just to demo the component working with a random block which supported the |
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? |
@@ -73,7 +85,7 @@ const ThemerComponent = () => { | |||
* saves edited entity data | |||
*/ | |||
const save = async () => { | |||
dispatch( 'core' ).undo(); | |||
// dispatch( 'core' ).undo(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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( |
@@ -98,6 +110,18 @@ const ThemerComponent = () => { | |||
); | |||
}; | |||
|
|||
/* TODO: refactor */ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@@ -108,13 +132,30 @@ const ThemerComponent = () => { | |||
|
|||
return ( | |||
<> | |||
<style>{ previewCss }</style> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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" /> |
<EditorContext.Provider | ||
value={ { | ||
globalStylesId, | ||
themeConfig, | ||
} } | ||
> | ||
<StylesContext.Provider | ||
value={ { | ||
setUserConfig, | ||
} } | ||
> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
</StylesContext.Provider> | ||
</EditorContext.Provider> | ||
{ /* demo */ } | ||
{ /* <TabPanel |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
config = set( config, selector, borders ); | ||
|
||
setUserConfig( config ); | ||
}, [ borders ] ); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Removed usage of useState for internal component state due to this state being unaffected by the global Reset button
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 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
This has been refactored to use the theme.json schema and theme.json styles properties as the source of truth for the Steps To TestBorder Component
Block List
Search
Change Log
Checklist
|
Adding test video: http://bigbite.im/v/qiLl1S |
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
Steps to test
Screenshots/Videos
Test video: http://bigbite.im/v/FCOHyl
Checklist: