Skip to content

Conversation

@rise-erpelding
Copy link
Contributor

@rise-erpelding rise-erpelding commented Oct 6, 2023

Description

Registers an additional sidebar area (primary_sidebar) to be used in the <aside> that was already built into the base.twig template. Adds minimal styling for the sidebar to visually distinguish it from other parts of the page.

Addresses #75

To Validate

  1. Make sure all PR Checks have passed
  2. Pull down this branch
  3. Run npm start
  4. Navigate to localhost:8000, confirm that no widgets appear in the footer or the sidebar initially (if you've already opened widgets before, reset the db)
  5. Navigate to wp-admin, then go to Appearance > Widgets. This automatically fills in registered widget areas with default content.
  6. Check localhost:8000 again to confirm that the widgets appear on the page and check sidebar styling.
  7. Review scss file additions to confirm that they make sense and follow conventions.

@rise-erpelding rise-erpelding force-pushed the support-multiple-sidebars branch from ffd80fb to 582e625 Compare October 6, 2023 16:42
@rise-erpelding rise-erpelding marked this pull request as ready for review October 6, 2023 16:49
Copy link
Contributor

@dustin-jw dustin-jw left a comment

Choose a reason for hiding this comment

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

Overall this looks pretty good and helps clarify how widgets are meant to be used, so well done! I just have some suggestions for naming and organization for clarity and a CSS refactor.

*/
function add_to_context( $context ) {
$context['menu'] = new \Timber\Menu( 'primary-menu' );
$context['primary_sidebar'] = Timber\Timber::get_widgets( 'primary-widget-area' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than requiring developers to know that they need to update this here when they add a widget, could we move a subset of this function into theme-widgets.php? It would be nice to have a sparkpress_add_widgets_to_context function in the same file that's clearly related to the widget registration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, WordPress has confusing naming (why are widgets called sidebars?), but we don't have to follow those confusing conventions. Can we call these sidebar_widget and footer_widget so it aligns better with what an admin would see in WordPress menus?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dustin-jw So we would rename and move (most of) this function to theme-widgets.php?

function add_to_context( $context ) {
         // don't move this line into theme-widgets
	$context['menu'] = new \Timber\Menu( 'primary-menu' );
	$context['primary_sidebar'] = Timber\Timber::get_widgets( 'primary-widget-area' );
	$context['footer_sidebar'] = Timber\Timber::get_widgets( 'footer-area' );
	return $context;
}
add_filter( 'timber/context', 'add_to_context' );

It feels like we could also do something similar with moving $context['menu'] by creating a function that adds it context in theme-setup.php, right? And if we did do that, do I take the add_to_context function out of functions.php altogether or leave some remnant of it in case future developers want to add things to it later?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if we left the menu part in add_to_context and left it in functions.php, that would be most useful for future devs, like it gives them an example of how to globally add something to $context in a place that they'll probably end up looking at at some point. Other than changing the theme name, I don't think there's much reason for a dev to interact with theme-setup.php, so I think that would obscure how the menu data gets into context.

register_sidebar(
array(
'name' => 'Primary Widget Area',
'id' => 'primary-widget-area',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd call this "Primary Sidebar" or something like that, especially since this one is pretty explicitly a sidebar.

Comment on lines 19 to 20
'before_title' => '<h3>',
'after_title' => '</h3>',
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I could tell, before_title and after_title don't appear to do anything, so we may be able to exclude them. I think they might have been used with the classic editor, but not with blocks.

'name' => 'Footer',
'id' => 'footer-area',
'description' => 'Sidebar for a the footer area.',
'description' => 'Sidebar for the footer area.',
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't actually know where the description gets shown, but can we call this "Widget for the footer area"?


@media (min-width: 50rem) {
margin: 0 0 0 3.75rem;
width: 18.75rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than setting margins and width, I think we can take advantage of flexbox features a little more.

In .cmp-post-body, you could use gap: 1.5rem 3.75rem; to achieve what the margins are doing, and then in .cmp-primary-sidebar, you could use flex-basis instead of width to set the ideal size while still allowing it to grow or shrink as needed.

@rise-erpelding rise-erpelding force-pushed the support-multiple-sidebars branch from 849320d to 3eeee9c Compare October 7, 2023 01:32
Copy link
Contributor

@dustin-jw dustin-jw 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, nice work!

- register and add widget areas to context in same file
- rename widgets/widget areas
- remove widget area titles
- adjust css for sidebar widget area
@dustin-jw dustin-jw force-pushed the support-multiple-sidebars branch from 3eeee9c to 11c0070 Compare October 9, 2023 16:37
@dustin-jw dustin-jw merged commit 4108f61 into main Oct 9, 2023
@dustin-jw dustin-jw deleted the support-multiple-sidebars branch October 9, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants