search

FortAwesome / wordpress-fontawesome

Font Awesome Official WordPress Plugin
Other
56 stars 19 forks source link

Update class-fontawesome.php #93

Closed ghost closed 3 years ago

ghost commented 3 years ago

Changes as proposed in #92

mlwilkerson commented 3 years ago

Actually, looks I was able to do the rebase. (Though it'll need to happen again, cause i just found a bug in the JavaScript dep updates I merged to master).

Still need to resolve the issue with the brand icon. If I can appear on the sub-menu, it seems like it should.

ghost commented 3 years ago

@mlwilkerson sorry if I messed things up a bit. As mentioned, it's all a bit new to me. Is there anything I could do differently next time in order to make your life easier?

As for the brand icon: I removed it intentionally, because the add_options_page() function does not take an icon parameter. As far as I know, there is no way to use an icon on a sub menu page in WP. So the icon lost its purpose, that's why I removed it.

mlwilkerson commented 3 years ago

@first-wgrt Yeah, I realized after posting my question about the brand icon that it didn't make sense to include it in a sub-menu. I'll want to double-check with a designer about the trade-off involved in that before finalizing this.

As for what to do differently next time, I think it was jus the > character in your branch name that I didn't realize was throwing me off when trying to check out the branch. I thought my git skillz were suddenly failing me. Turns out, it was just that I didn't notice at first that my shell was interpreting the > as output redirection unless I put quotation marks around the branch name. So I'd suggest naming branches with only alpha, numeric, or hyphen (-) characters.

Finally, you may notice now that there is a CI failure on coding style if you look at the details, you'll see:

FILE: ...fontawesome/wordpress-fontawesome/includes/class-fontawesome.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 764 | ERROR | [x] Tabs must be used to indent lines; spaces are not
     |       |     allowed
     |       |     (Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

That's probably the last thing that needs fixed.

ghost commented 3 years ago

@mlwilkerson Thanks for the feedback, that's helpful!

I fixed the PHPCS error, all checks passed 😀