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

Header template #74

Merged
merged 3 commits into from
Dec 18, 2023
Merged

Header template #74

merged 3 commits into from
Dec 18, 2023

Conversation

nathan-schmidt-viget
Copy link
Contributor

@nathan-schmidt-viget nathan-schmidt-viget commented Dec 13, 2023 •
edited
Loading

Summary

This creates a header part that matches the style of the designs.
@clatwell the button icon is not added, we need to figure out if we need to build that out in WP (or just use an image like we talked about), and if we do then let's do a separate issue to add icons to all buttons.

As part of matching the header, we need to have the Goodbids logo be the default logo on a new site. This was a bit more complex than I was originally thinking.

This PR is dependent on the PR

The process is:

  • A new site is created
  • Copy the site_logo from the main site
  • Upload the image to the new site
  • Set that new image as the site_logo

Issues

Testing Instructions

Add a logo

  • Go to the main GoodBids site and set a site logo
    • Appearance -> Editor
    • Patterns -> Template Parts -> Header
    • Edit the header and add an image to the site logo
    • Screenshot 2023-12-13 at 12 37 02 PM

Create a new site

  • Go to the Network Admin -> Sites and create a new site
  • Enter all the info
  • Go to the new site URL
  • Make sure there is the copied image in the logo area in the header

Screenshots

Designs

Screenshot 2023-12-13 at 11 23 38 AM

Site

Screenshot 2023-12-13 at 1 37 14 PM

@nathan-schmidt-viget nathan-schmidt-viget marked this pull request as ready for review December 13, 2023 20:44
@clatwell
Copy link
Contributor

@nathan-schmidt-viget - I'll spin up a separate polish ticket to add the icon to the Login button!

nathan-schmidt-viget
nathan-schmidt-viget commented Dec 13, 2023
View reviewed changes
themes/goodbids-nonprofit/assets/css/theme.css
outline-style: dotted;
outline-width: 1px;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Planning on moving this all to tailwind.

nathan-schmidt-viget
nathan-schmidt-viget commented Dec 13, 2023
View reviewed changes
themes/goodbids-nonprofit/parts/header.html
<!-- /wp:group -->
</div>
<!-- /wp:group -->
<!-- wp:pattern {"slug":"goodbids-np/header-nonprofit"} /-->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this to a pattern, so if we need to use PHP we can.

nathanlong
nathanlong approved these changes Dec 14, 2023
View reviewed changes
Copy link

@nathanlong nathanlong left a comment

Choose a reason for hiding this comment

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

This seems like a great quality of life improvement for editors rather than having them upload the logo every time a new child site is created.

@nathan-schmidt-viget nathan-schmidt-viget self-assigned this Dec 14, 2023
bd-viget
bd-viget suggested changes Dec 14, 2023
View reviewed changes
Copy link
Contributor

@bd-viget bd-viget left a comment

Choose a reason for hiding this comment

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

Left a comment about the copy logo approach, but the rest of the changes look good!

client-mu-plugins/goodbids/src/classes/network/Sites.php Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple of thoughts on this approach:

  1. This seems like quite a few steps that need to execute when we're creating a new site:
    1. Switch to main site
    2. Retrieve the logo
    3. Validate the logo file path
    4. Switch back to original site
    5. Copy the logo file to the new site
    6. Create an attachment post
    7. Update the new site logo field
  2. I'm nervous about using copy(), especially if we need the @ - Let's try to avoid using @ at all costs.
  3. If the GoodBids main site ever changes/updates their logo, we're going to end up with many artifacts of the old logo that will be hard to update/change/remove.

My recommended approach would be to add a filter using get_custom_logo (or a similar hook), and simply overriding the logo with whatever logo we have on the main site if they have not set up one on their site. Would that be possible? Let me know if you want to pair on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bd-viget ya lets pair on this. I went down the path of setting the logo to a default image but then ran into a problem with it being an image and not the logo block.
I am open this afternoon or tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I think this was a bit of confusion on my understanding. I had tried a filter, but it was with filtering it on the page editor led to my problem. I looked through what you posted about get_custom_logo and updated the code.

Base automatically changed from bd/21-user-authentication to main December 15, 2023 14:10
bd-viget
bd-viget approved these changes Dec 15, 2023
View reviewed changes
Copy link
Contributor

@bd-viget bd-viget 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 this is great! I left a comment about one more direction we could take here, but, it may be worth putting into a future ticket. This will work totally fine!

client-mu-plugins/goodbids/src/classes/Network/Sites.php
return sprintf(
'<a href="%1$s" class="custom-logo-link" rel="home" itemprop="url"><img src="%2$s" class="custom-logo" itemprop="logo" alt="GoodBids"></a>',
esc_url( home_url( '/' ) ),
esc_attr( GOODBIDS_PLUGIN_URL . 'assets/images/goodbids-logo.png' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is totally fine, however, what are your thoughts about retrieving the URL (or Markup) of the Main site custom_logo and rendering that here? So if they upload a new logo to the Main site, this will inherit that new logo without requiring a change to the plugin or repo. If you look into it, be sure to do an is_main_site() check otherwise you could end up in an infinite loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a great idea. I am going to keep it as is for now, but will make a note of it.

@nathan-schmidt-viget
[#12] styling header navigation with logout button
48030ca 
[#12] updating header part
[#12] adding site default logo function
[#12] setting up default logo
[#12] adding logo copy function
[#12] update site logo
@nathan-schmidt-viget
[#12] refactor header part to header pattern
10c6646
@nathan-schmidt-viget
[#12] updating default logo to filter
33e0551
@nathan-schmidt-viget nathan-schmidt-viget merged commit df8a42f into main Dec 18, 2023
1 check passed
@nathan-schmidt-viget nathan-schmidt-viget deleted the ns/12-header branch December 18, 2023 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bd-viget bd-viget bd-viget approved these changes

@nathanlong nathanlong nathanlong approved these changes

@ten1seven ten1seven Awaiting requested review from ten1seven

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

Successfully merging this pull request may close these issues.
4 participants
@nathan-schmidt-viget @clatwell @nathanlong @bd-viget