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

new icon: rails (plain, plain-wordmark) #554

Merged
merged 8 commits into from
Apr 9, 2021

Conversation

maltejur
Copy link
Contributor

@maltejur maltejur commented Apr 8, 2021

Old:
Screenshot 2021-04-08 205802

New:
Screenshot 2021-04-08 205829

This is how their website now looks:
Screenshot 2021-04-08 205906

@github-actions

This comment has been minimized.

@Panquesito7 Panquesito7 added the feature:icon Use this label for pull requests when a new icon is ready to be added to the collection label Apr 8, 2021
@Panquesito7 Panquesito7 added the bot:peek Use this label to trigger peek-bot. Remove and re-add the label to re-trigger label Apr 8, 2021
@Panquesito7 Panquesito7 self-requested a review April 8, 2021 19:26
@github-actions

This comment has been minimized.

@Panquesito7
Copy link
Member

Hmm, this is failing because of the commit title. See https://github.com/devicons/devicon/blob/develop/CONTRIBUTING.md#overview for more information.

@maltejur maltejur changed the title update icon: rails (plain, plain-wordmark) new icon: rails (plain, plain-wordmark) Apr 8, 2021
@Panquesito7
Copy link
Member

I think it's only with the commit title. Once I tried renaming the PR, re-running the workflow, and it didn't work. It only worked by changing the commit title.

@github-actions

This comment has been minimized.

@Panquesito7 Panquesito7 added bot:peek Use this label to trigger peek-bot. Remove and re-add the label to re-trigger and removed bot:peek Use this label to trigger peek-bot. Remove and re-add the label to re-trigger labels Apr 9, 2021
@github-actions

This comment has been minimized.

@Thomas-Boi
Copy link
Member

I think I know what went wrong. The peek script only checks icons that haven't been added to to icomoon.json. Since we do have a current rails entry, the script is not seeing the new entry.

I'm looking into a possible fix. The easiest one I can think of is removing the rails entry in the icomoon.json

@Thomas-Boi
Copy link
Member

@maltejur the only fix would be to modify the icomoon.json unfortunately. There are two ways to do so, either edit it in the json or change it using the Icomoon website.

Edit it in the JSON:

  • Open the icomoon.json in your code editor. You might have to format it cause the file is truncated.
  • Find the entry for rails and delete ONLY those entries.
  • Save the file, commit and push again.

Edit it using the Icomoon Website:

  • Go to this page

  • In the top left corner, there's a Import Icons button. Click on it and find the icomoon.json in the your file system.

  • This will load all the icons from the project into Icomoon. There might be a popup: accept "Yes" for config options.
    image

  • Use the search box to find the rails icons. Delete them.
    image

  • At the bottom right, there's a Generate Font button. Click on it.
    image

  • There might be a popup talking about strokes and fills. Press "Continue"
    image

  • Click "Download"
    image

-This will download a zip file to your computer. Open it and select only the selection.json.
-Rename the file to icomoon.json. Add this new one to your local repo, commit, then push again.

Please try those steps. I'm 99% sure that they'll work.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 9, 2021

Hi!
I'm Devicons' SVG-Checker Bot and everything looks great. Good job!

Have a nice day,
SVG-Checker Bot 😁

@Panquesito7 Panquesito7 added bot:peek Use this label to trigger peek-bot. Remove and re-add the label to re-trigger and removed bot:peek Use this label to trigger peek-bot. Remove and re-add the label to re-trigger labels Apr 9, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Apr 9, 2021

Hi there,

I'm Devicons' Peek Bot and I just peeked at the icons that you wanted to add using icomoon.io.
Here is the result below:

Imgur Images

Here are the zoomed-in screenshots of the added icons:
Imgur ImagesImgur Images

Note: If the images don't show up, it's probably because it has been autodeleted by Imgur after 6 months due to our API choice.

The maintainers will now take a look at it and decide whether to merge your PR.

Thank you for contributing to Devicon! I hope everything works out and your icons are accepted into the repo.

Cheers,
Peek Bot 😊

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.

LGTM. Thank you for your contribution! 👍 🎉

@Panquesito7 Panquesito7 requested a review from Thomas-Boi April 9, 2021 18:25
Copy link
Member

@Thomas-Boi Thomas-Boi left a comment

Choose a reason for hiding this comment

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

This looks great now. Thanks @maltejur for putting in the extra work.

I'll look into a solution so this won't happen again in the future.

@Thomas-Boi Thomas-Boi merged commit ce558b1 into devicons:develop Apr 9, 2021
@maltejur
Copy link
Contributor Author

maltejur commented Apr 9, 2021

Thanks for the detailed fix @Thomas-Boi

From https://github.com/devicons/devicon/blob/master/.github/scripts/icomoon_peek.py:

args = arg_getters.get_selenium_runner_args(True)
new_icons = filehandler.find_new_icons(args.devicon_json_path, args.icomoon_json_path)

# get only the icon object that has the name matching the pr title
filtered_icons = find_object_added_in_this_pr(new_icons, args.pr_title)

if len(new_icons) == 0:
    sys.exit("No files need to be uploaded. Ending script...")

if len(filtered_icons) == 0:
    message = "No icons found matching the icon name in the PR's title.\n" \
    "Ensure that a new icon entry is added in the devicon.json and the PR title matches the convention here: \n" \
    "https://github.com/devicons/devicon/blob/master/CONTRIBUTING.md#overview.\n" \
    "Ending script...\n"
    sys.exit(message)

# print list of new icons
print("List of new icons:", *new_icons, sep = "\n")
print("Icons being uploaded:", *filtered_icons, sep = "\n", end='\n\n')

runner = None
try:
    runner = SeleniumRunner(args.download_path, args.geckodriver_path, args.headless)
    svgs = filehandler.get_svgs_paths(filtered_icons, args.icons_folder_path, True)
    screenshot_folder = filehandler.create_screenshot_folder("./") 
    runner.upload_svgs(svgs, screenshot_folder)
    print("Task completed.")

Why doesn't the find_object_added_in_this_pr function just take all the icons?

@amacado amacado mentioned this pull request Apr 10, 2021
GCHQDeveloper926 pushed a commit to GCHQDeveloper926/devicon that referenced this pull request Dec 20, 2024
new icon: rails (plain, plain-wordmark)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:peek Use this label to trigger peek-bot. Remove and re-add the label to re-trigger feature:icon Use this label for pull requests when a new icon is ready to be added to the collection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants