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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

add new icon OBSERVABLE #281

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

mattdzugan
Copy link

Proposing a new icon for Observable

image

had to create a second PR cuz i broke travis the first time 馃槵

@shinenelson shinenelson added the icon-request New icon request label Sep 4, 2021
Copy link
Member

@shinenelson shinenelson left a comment

Choose a reason for hiding this comment

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

@mattdzugan, thank you for adding the icon for Observable to Fork Awesome. It looks good 馃憣

However, in order for these changes to be merged in, can you please remove the generated font files from fonts and also src/icons/.fontcustom-manifest.json?

The duplication of package.json in src/icons is not required as well. Also, please resolve the merge conflict in src/icons/icons.yml by accepting both changes.

@mattdzugan
Copy link
Author

mattdzugan commented Sep 4, 2021

Will do thank you @shinenelson !!
I'll also try and update the contributing.md to make it more clear to folks like me exactly which files to include in the PRs

@shinenelson
Copy link
Member

I'll also try and update the contributing.md to make it more clear to folks like me exactly which files to include in the PRs

That would be great. It would be appreciated if you could create a separate pull request for it. We try and keep pull requests concise with what it does.

The reasoning behind that is because we follow a squash merge strategy and it would make sense to have only one item per pull request.

@ForkAwesome ForkAwesome locked and limited conversation to collaborators Sep 4, 2021
@ForkAwesome ForkAwesome unlocked this conversation Sep 4, 2021
@shinenelson
Copy link
Member

I am sorry about the locking. It was an accidental tap on mobile 馃槄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
icon-request New icon request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants