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

Feature: Peek and Build-Bot Upgrade #806

Merged
merged 22 commits into from
Aug 13, 2021
Merged

Conversation

Thomas-Boi
Copy link
Member

Hello guys,

This is a big upgrade of our build/peek workflow. I thought of splitting it up into multiple PRs but there are a lot of similar aspects (peek-bot and build-bot has the same superclass, the footer upgrade is a part of the build-bot). I apologize in advance if this is a lot and feel free to take your time to review this.

Here is a list of features according to categories:

peek-bot:

  1. Feature:
    • Now let us know whether an icon contains strokes when uploading them to Icomoon. This is a secondary check to an upcoming checkSvg update
    • Take snapshots of the ICONS version generated by Icomoon. The images seen on the "Generate Font" page of Icomoon is actually what the final font version look like. peek-bot now takes a picture of that.
    • Improved messaging to display the images
    • PR title can now be either "new icon" or "update icon". This is merely a semantic change so it looks clearer in the PR tab and release message
  2. Things maintainer should know:
    • The glyph counts should match the number of icons selected (see the peek-bot's PR message, I added a list of things we should look out for). If not, something went wrong => could be because color is not stripped properly.
  3. Test:
    • See this PR to see a test case where there are NO STROKES in the SVG.
    • See this PR to see a test case where there are STROKES in the SVG.

build-bot:

  1. Feature:

    • Now takes picture of the icons generated by Icomoon
    • Also checks for strokes in SVGs and duplicated icons. If these happen, build script will fail.
    • Incorporate get_release_message workflow into the build-bot. We don't have to run the two scripts separately anymore.
    • optimizer-bot (a part of build-bot):
      • Will append a \n to each SVG after it's optimized (looking at you *nix users)
      • Will prefix the classes -> SVGs can now use classes and <style> tags with no issues.
  2. Things maintainer should know:

    • The build release message is now included in the PR message. You can copy it by hovering over the top right corner of the release message section.

    • image

    • Please check the files via "Edit Files" to see the \n in the SVG files. For some reason, GitHub don't display them but they are there. You can download the file and check locally as well => the \n will be there.

    • image

    • image

  3. Test:

    • See this PR to see the built-bot's result.
    • Here's the Actions log for it.

Both the upgrades require merge into master due to the workflow_run events in the related workflow files.

After this PR, I have another one for checkSvg. After that, we can create a new release.

@Thomas-Boi Thomas-Boi added devops Use this label for devops related enhancements enhancement labels Aug 12, 2021
amacado
amacado previously approved these changes Aug 13, 2021
Copy link
Member

@amacado amacado left a comment

Choose a reason for hiding this comment

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

This is crazy! 👍🏻 @Thomas-Boi amazing how much effort you bring in!

Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

I don't know anything about Python and I'm not that experienced in this area, but overall, everything looks amazing! Awesome work, @Thomas-Boi! 😄

filehandler.extract_files(str(zip_path), args.download_path)
filehandler.rename_extracted_files(args.download_path)

print("Creating the release message by querying GitHub API...")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
print("Creating the release message by querying GitHub API...")
print("Creating the release message by querying the GitHub API...")

Thank you for contributing to Devicon! I hope everything works out and your icons are accepted into the repo.
In case of font issues, it might be caused by Icomoon not accepting strokes in the SVGs. Check this [doc](https://icomoon.io/#faq/importing) for more details and fix the issues as instructed by Icomoon and update this PR once you are done.

Thank you for contributing to Devicon! I hope that your icons are accepted into the repo.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Thank you for contributing to Devicon! I hope that your icons are accepted into the repo.
Thank you for contributing to Devicon! I hope that your icons are accepted into the repository.

.gitignore Outdated
new_icons.png
screenshots/
release_message.txt
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
release_message.txt
release_message.txt

gulpfile.js Outdated
@@ -21,7 +23,7 @@ async function createDeviconMinCSS() {
await createCSSFiles();

let deviconMinPath = path.join(__dirname, finalMinSCSSName);
// recall that devicon-alias.scss imported the devicon.css => don't need
// recall that devicon-alias.scss imported the devicon-icomoon.css => don't need
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
// recall that devicon-alias.scss imported the devicon-icomoon.css => don't need
// recall that devicon-alias.scss imported the devicon-base.css => don't need

@Thomas-Boi
Copy link
Member Author

Two things I forgot to mentioned:

  • I renamed the devicon.css into devicon-base.css. People keep thinking that the devicon-min.css is an optimized version of that, which is only half-correct. Hopefully, a rename will make it clearer that this is not the case.
  • The images in the build-bot test example were reversed. This has been fixed in e72f344.

Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

As I said previously, I don't know anything about Python or related to this area, but the changes mentioned in the first comment look good. Huge thanks and awesome work, @Thomas-Boi! 😄

@Thomas-Boi
Copy link
Member Author

Thank you for reviewing the changes! This took me quite a while and I'm happy that everything went well. We can start making a new release and putting the bot to work 😄

@Thomas-Boi Thomas-Boi merged commit 08aa325 into develop Aug 13, 2021
@Panquesito7 Panquesito7 deleted the thomas/features/seleniumUpdate branch August 13, 2021 19:43
@amacado amacado mentioned this pull request Aug 14, 2021
GCHQDeveloper926 pushed a commit to GCHQDeveloper926/devicon that referenced this pull request Dec 20, 2024
* Refactored peek script into a class

* Post-peek workflow now upload the new screenshots

* Refactored BuildSeleniumRunner into a class

* Updated build_icons.yml to reflect new changes

* Fixed issue with building icons that were already in the app

* Build script will take screenshot of new icons

* Update post peek yaml message

* Added alerts

* Peek script now check for strokes in icons

* Updated post_peek's strokes in svgs message

* Updated post_peek script's message

* Updated post_peek's message

* Refactored get_release_message into icomoon_build

* Change devicon.css name to devicon-base.css

* Updated post_peek message

* Added update icon as a valid PR title for bot-peek

* Add \n char to SVG after it gets optimized

* Fixed error with 'update icon' regex

* Build script now batch issues when upload SVG

* Addressed build-bot's screenshot order

* Apply suggestions from code review

Co-authored-by: David Leal <[email protected]>

Co-authored-by: David Leal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops Use this label for devops related enhancements enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants