[Wordpress] Author & Contributor Badges #5724
Conversation
Added author badges for plugin and theme, and contributor badge for plugins
Fixed platform test mocked responses. Also includes mocked data for another PR as a comment
|
|
I'd still strongly prefer these services be split out into separate files that are better named to reflect the noun of the badge as opposed to have a category driven file name that contains various badge classes that utilize that category (split out author and contributors into separate files) I feel like this will be more aligned with our typical structure and thus more easy to debug and maintain long term |
|
@calebcartwright I will split them out, I think your right, it was a bit confusing finding where certain badges are compared to other services. Is this something we need to consider on the other PR's to bring the service as a whole inline with the rest? |
Just to clarify, are you referring to changing the existing class/file structure for those already in the repo, or the other Wordpress PRs you have opened that are adding net-new badges? If it's the former, I'm not entirely sure which ones you're referring to. I suppose there's a case that could be made that the |
|
I just meant is there any other similar to this within WordPress and my other PR's we could address at the same time. Yeah I can see why avoiding names like that is probably for the best! |
Gotcha. I think everything else is fine, though the rest could always be reviewed and discussed later in a separate PR. |
There was a problem hiding this comment.
I've left a few inline comments around the implementation, but have to say I'm not really sure what the intent/benefit of these badges are and I'm not convinced they are a worthwhile addition (at least in the current state).
My assumption when I saw a reference to a Contributor badge was that it would be displaying the count of contributors to a given plugin or theme, however, I was clearly wrong about that.
While I do see value in a contributor count badge, I do not see any value in a badge that simply displays the contributor name with a check to see whether that is true.
If someone really wants to display their username then they can just do so with our static badge service anyway, and the same would be true for an author.
I could potentially see a Contributor Count badge, but I'm a "no vote" on dynamic badges that display the author name or a contributor/user name
| transform(rawAuthor) { | ||
| return rawAuthor.substring( | ||
| rawAuthor.lastIndexOf('">') + 2, | ||
| rawAuthor.lastIndexOf('<') | ||
| ) | ||
| } |
There was a problem hiding this comment.
@chris48s - so this is a great example of the hypothetical scenario I referred to in #5722 (comment) (the schema is just a Joi.string().required())
I do feel strongly that somewhere we need to have some kind of validation since we need to slice and dice the string value, whether we should do this as part of transform and throwing a more contextual error message or as part of the schema is where there's still some questions IMO.
There was a problem hiding this comment.
This is part of the service I do need to revisit actually as I realised it may not work all the time.
The string can contain a HTML link, which is what the transform is stripping out, leaving us with just the name of the author. However, if no link is set, I believe it should just return the name, no HTML. So I will need to rework this to detect and deal with the HMTL and without correctly.
There was a problem hiding this comment.
Agreed. It makes sense to check the string contains < [some sensible number of characters] and a > before we try and parse it like it this, otherwise we're going to get unexpected outputs. Using a .regex() in the schema is probably the sensible place to do that.
Are we sure the string can contain only one HTML element?
| const { author: rawAuthor } = await this.fetch({ | ||
| extensionType, | ||
| slug, | ||
| }) | ||
| const author = rawAuthor.display_name | ||
| const profile = rawAuthor.profile | ||
| return this.constructor.render({ author, link: profile }) |
There was a problem hiding this comment.
Don't feel obligated to modify this, but just noting for awareness that we could leverage more deconstructing to make this a bit more succinct
| const { author: rawAuthor } = await this.fetch({ | |
| extensionType, | |
| slug, | |
| }) | |
| const author = rawAuthor.display_name | |
| const profile = rawAuthor.profile | |
| return this.constructor.render({ author, link: profile }) | |
| const { author: { display_name: author, profile: link } } = await this.fetch({ | |
| extensionType, | |
| slug, | |
| }) | |
| return this.constructor.render({ author, link }) |
|
I agree, the contributors badge is basically pointless, only automating adding the contributors link which isn't really worth it. I will look to see if something like a contributor count is possible, should be an easy matter of counting entries in the contributor array. |
Awesome thanks! Please note that the same issue exists with the author badge for me. We really don't want to add extra work and API calls to our badge servers simply to make it easier for users to create what are tantamount to static badges. If someone really wants a social-styled badge that displays their profile and includes a link, then they should just use the static badge feature |
|
Similarly to #5721, given that there hasn't been any activity for a while, I'm going to close this pull request. Feel free to ping us or reopen it if you get a chance to look into it again. 😉 If someone else wants to pick this up, you can simply fetch the commits by running |
Related to #5493, this PR adds Author and Contributor social badges for Wordpress themes and plugins.
Also included is some mock data, which will be needed for related PR #5723, specifically its new mock test in platform testing.