-
Notifications
You must be signed in to change notification settings - Fork 27
refactor: moved classic sidebar settings #1127
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
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.
Pull Request Overview
This refactor changes the conditional rendering approach for sidebar settings in the Feedzy block inspector, replacing React conditional rendering with CSS display toggling to move classic sidebar settings to the top.
- Replaced conditional JSX rendering (
{condition && <Component />}
) with div wrappers using CSS display properties - Modified three main sections: content tab, style tab, and advanced tab rendering
- Changed from React's conditional mounting/unmounting to CSS visibility control
js/FeedzyBlock/inspector.js
Outdated
@@ -96,7 +96,7 @@ class Inspector extends Component { | |||
</span> | |||
</Button> | |||
</PanelBody> | |||
{'content' === this.state.tab && ( | |||
<div style={{ display: 'content' === this.state.tab ? 'block' : 'none' }}> |
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.
Using CSS display toggle keeps all components mounted in the DOM even when hidden, which can impact performance compared to conditional rendering. The original approach ({condition && <Component />}
) only renders components when needed, reducing memory usage and improving performance.
<div style={{ display: 'content' === this.state.tab ? 'block' : 'none' }}> | |
{this.state.tab === 'content' && ( |
Copilot uses AI. Check for mistakes.
js/FeedzyBlock/inspector.js
Outdated
|
||
{'fetched' === this.props.state.route && | ||
'style' === this.state.tab && ( | ||
<div 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.
[nitpick] The inline style object with complex conditional logic spanning multiple lines reduces readability. Consider extracting this logic into a computed property or helper function for better maintainability.
Copilot uses AI. Check for mistakes.
js/FeedzyBlock/inspector.js
Outdated
<div style={{ | ||
display: 'fetched' === this.props.state.route && | ||
'advanced' === this.state.tab ? 'block' : 'none' |
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.
[nitpick] Similar to the previous comment, this complex conditional logic in inline styles makes the code harder to read and maintain. Consider consolidating the display logic into reusable helper functions.
<div style={{ | |
display: 'fetched' === this.props.state.route && | |
'advanced' === this.state.tab ? 'block' : 'none' | |
<div style={{ | |
display: this.getDisplayStyle( | |
this.props.state.route, | |
this.state.tab, | |
'fetched', | |
'advanced' | |
) |
Copilot uses AI. Check for mistakes.
Plugin build for 76b1392 is ready 🛎️!
Note You can preview the changes in the Playground |
868a76a
to
d2fe022
Compare
d2fe022
to
fec15b0
Compare
Summary
Moved Feedzy classic sidebar settings to the top
Will affect visual aspect of the product
YES
Screenshots
Closes https://github.com/Codeinwp/feedzy-rss-feeds-pro/issues/863