close
The Wayback Machine - https://web.archive.org/web/20220720233242/https://github.com/WordPress/performance/pull/231
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

@felixarntz felixarntz commented Mar 15, 2022

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 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

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

@jeffpaul
Copy link
Member

@jeffpaul jeffpaul commented Mar 15, 2022

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

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

@felixarntz felixarntz commented Mar 15, 2022

@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

@jeffpaul jeffpaul commented Mar 15, 2022

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 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?

@felixarntz felixarntz requested a review from jeffpaul Mar 15, 2022
@jeffpaul
Copy link
Member

@jeffpaul jeffpaul commented Mar 16, 2022

@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

Hooray, beautiful branding!

@felixarntz
Copy link
Member Author

@felixarntz felixarntz commented Mar 16, 2022

@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

@felixarntz felixarntz commented Mar 17, 2022

@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 Mar 17, 2022
@felixarntz felixarntz changed the base branch from trunk to release/1.0.0-beta.2 Mar 18, 2022
@rwlands
Copy link

@rwlands rwlands commented Mar 18, 2022

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

@rwlands
Copy link

@rwlands rwlands commented Mar 18, 2022

Icon_Black
Icon_Blue
Icon_White

mitogh
mitogh approved these changes Mar 18, 2022
@mitogh
Copy link
Member

@mitogh 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 🎉

mitogh
mitogh approved these changes Mar 18, 2022
@felixarntz
Copy link
Member Author

@felixarntz felixarntz commented Mar 18, 2022

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
3 checks passed
@felixarntz felixarntz deleted the add/plugin-assets branch Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure [Type] Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants