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.

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

Add plugin banner and icon assets #231

Merged
merged 4 commits into from Mar 18, 2022
Merged

Conversation

felixarntz
Copy link
Member

Summary

Fixes #144

Relevant technical choices

  • Assets in the new .wordpress-org folder will automatically deployed with every release tag via our GitHub workflow.

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@felixarntz felixarntz added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure labels Mar 15, 2022
@felixarntz felixarntz added this to the 1.0.0-beta.2 milestone Mar 15, 2022
Copy link
Member

@jeffpaul jeffpaul left a comment

Choose a reason for hiding this comment

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

you'll also want to add the .wordpress-org directory to the .gitattributes file

@jeffpaul
Copy link
Member

Also, please ensure that you prop @rwlands and any others from the source issue so that we're properly crediting folks for this effort

Copy link
Member

@mitogh mitogh left a comment

Choose a reason for hiding this comment

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

My only concern is the SVG icon which is almost 8 MB in size, it seems like the SVG is including nonrequired stuff. It would be great if we can have a smaller size created out of geometric shapes instead of actual images.

@felixarntz
Copy link
Member Author

@jeffpaul

you'll also want to add the .wordpress-org directory to the .gitattributes file

Ah right, that makes sense from a GitHub perspective. Just double-checking, will the 10up deploy action still consider the directory for the asset deployment though if it's "ignored" via .gitattributes?

Also, please ensure that you prop @rwlands and any others from the source issue so that we're properly crediting folks for this effort

Absolutely, however right now we have no process at all for any props for this plugin. This would be mostly relevant for eventual WP core merges for sure, but how about during plugin development? How do other feature plugins handle that? If there's something we can do here I'd say let's open an issue to put the necessary guidelines / tooling in place.

@mitogh

My only concern is the SVG icon which is almost 8 MB in size, it seems like the SVG is including nonrequired stuff. It would be great if we can have a smaller size created out of geometric shapes instead of actual images.

Good catch! Can you potentially look into that @rwlands? Indeed the SVG file is extremely large at the moment.

@jeffpaul
Copy link
Member

Ah right, that makes sense from a GitHub perspective. Just double-checking, will the 10up deploy action still consider the directory for the asset deployment though if it's "ignored" via .gitattributes?

from the action readme, which also matches my experience with the action on 10up's plugins:

This Action commits the contents of your Git tag to the WordPress.org plugin repository using the same tag name. It can exclude files as defined in either .distignore or .gitattributes, and moves anything from a .wordpress-org subdirectory to the top-level assets directory in Subversion (plugin banners, icons, and screenshots).

Absolutely, however right now we have no process at all for any props for this plugin. This would be mostly relevant for eventual WP core merges for sure, but how about during plugin development? How do other feature plugins handle that? If there's something we can do here I'd say let's open an issue to put the necessary guidelines / tooling in place.

Ah right, yeah I suppose we should get something in place soon before it becomes unwieldy to determine who to prop if (🤞🏼 when 🤞🏼) the plugin merges to core. 10up tends to utilize a CREDITS.md file (example) to keep track of those folks across versions as well as proping them on changelog items. I could help go back through things already merged and gather those props into the changelog and/or credits file, but curious which approach (or both) you would prefer to help track things ahead of a core merge?

@felixarntz
Copy link
Member Author

felixarntz commented Mar 15, 2022

@jeffpaul

from the action readme, which also matches my experience with the action on 10up's plugins

Sounds good, updated!

Ah right, yeah I suppose we should get something in place soon before it becomes unwieldy to determine who to prop if (🤞🏼 when 🤞🏼) the plugin merges to core. 10up tends to utilize a CREDITS.md file (example) to keep track of those folks across versions as well as proping them on changelog items. I could help go back through things already merged and gather those props into the changelog and/or credits file, but curious which approach (or both) you would prefer to help track things ahead of a core merge?

A credits file would help, however I think ours may need to be more granular than the one you've linked. There won't be a moment where "the plugin merges to core", since this is multiple WP core performance features in different stages of development, and they would merge individually when they're ready respectively. So to best support this workflow we would probably need some sort of credits file that is broken down by the different modules in this plugin - which is still possible, but a bit more involved.

Would you mind creating an issue so we can explore this further?

@jeffpaul
Copy link
Member

@felixarntz to further clarify my understanding before opening the issue, perhaps a credits file within the sub-directories in https://github.com/WordPress/performance/tree/trunk/modules would suffice for your concern?

Otherwise this PR looks ✅ to me.

Copy link
Member

@jeffpaul jeffpaul left a comment

Choose a reason for hiding this comment

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

Hooray, beautiful branding!

@felixarntz
Copy link
Member Author

@jeffpaul

@felixarntz to further clarify my understanding before opening the issue, perhaps a credits file within the sub-directories in https://github.com/WordPress/performance/tree/trunk/modules would suffice for your concern?

That's an alternative option, yes. I think we should discuss that in the eventual issue, whether it's preferable to have

  • either a single CREDITS.md file in the repository root, with contributors broken down per module
  • or a CREDITS.md file for every module in the module's directory

@felixarntz
Copy link
Member Author

@mitogh I just removed the icon.svg for now since it's too large as you've pointed out. I'd say we can already proceed with the other banner and icon assets since they are the essentials of what's needed really. Many plugins do not provide any icon.svg, so it's okay to add it later I'd say. This way we can still release with the new assets next week.

@felixarntz felixarntz requested a review from mitogh March 17, 2022 19:29
@felixarntz felixarntz changed the base branch from trunk to release/1.0.0-beta.2 March 18, 2022 00:57
@rwlands
Copy link

rwlands commented Mar 18, 2022

Hi there, I can look into the svg size issue, that's certainly very overweight!

@rwlands
Copy link

rwlands commented Mar 18, 2022

Icon_Black
Icon_Blue
Icon_White

@mitogh
Copy link
Member

mitogh commented Mar 18, 2022

Sounds good @felixarntz looks like a new SVG was added tho in case we want to add those as well.

Thank you @rwlands for getting these SVG icons for us, great work 🎉

@felixarntz
Copy link
Member Author

Thanks so much for fixing the SVG @rwlands! This is now good to be merged 🎉

@felixarntz felixarntz merged commit cd2f4e8 into release/1.0.0-beta.2 Mar 18, 2022
@felixarntz felixarntz deleted the add/plugin-assets branch March 18, 2022 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Issues for the overall performance plugin infrastructure [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance Lab: Branding
4 participants