Skip to content

Adding Diwali assets#96

Merged
lemonsaurus merged 2 commits into
python-discord:masterfrom
SurajBhari:master
Mar 13, 2021
Merged

Adding Diwali assets#96
lemonsaurus merged 2 commits into
python-discord:masterfrom
SurajBhari:master

Conversation

@SurajBhari
Copy link
Copy Markdown
Contributor

No description provided.

@ghost ghost added the needs 2 approvals label Nov 10, 2020
@ghost
Copy link
Copy Markdown

ghost commented Nov 10, 2020

Thank you for contributing to Python Discord!

Please check out the following documents:

Copy link
Copy Markdown
Contributor

@gustavwilliam gustavwilliam left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution! I have viewed the pngs and they look good. Do we need all of these sizes, though?

I see you've added the same files in multiple places. I think it would be nice to have the same file in only one place, so it's easier to maintain. Since I both assume we won't be modifying this later on and appreciate having them accessible in both logical places, I won't request changes on this. It's more of a general comment about having it in multiple places and mentioning that it would be nice to have the exact same file in only one place.

@SurajBhari
Copy link
Copy Markdown
Contributor Author

I took the reference from easter directory. as shown it also had the same layout I decided to go with same things.
I don't think there is any issue having multiple sizes tho.

@Akarys42
Copy link
Copy Markdown
Contributor

Well, one of the downside would be the repo size, if we don't need the different sizes, then why should we have them? We can rerender the svg at any time.

Although, no need to delete them now, since they are in the history. It isn't a big deal anyway.

I really like the assets, very good job!

@gustavwilliam
Copy link
Copy Markdown
Contributor

Okay. It'll probably be fine to store them in multiple places this time.

@Akarys42 yes, that's exactly the reason for including only the necessary versions. If any new ones are needed, there are (hopefully) source files to generate them from. That may be something to keep in mind in the future.

gustavwilliam
gustavwilliam previously approved these changes Nov 10, 2020
Copy link
Copy Markdown
Contributor

@gustavwilliam gustavwilliam left a comment

Choose a reason for hiding this comment

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

Great work! I like it.

@ghost ghost removed the needs 1 approval label Nov 10, 2020
@SebastiaanZ
Copy link
Copy Markdown
Contributor

Where did you find the assets for these icons?

@SebastiaanZ SebastiaanZ dismissed gustavwilliam’s stale review November 10, 2020 17:36

We'll need to make sure the licenses are in order before merging something into branding

@ghost ghost added the needs 1 approval label Nov 10, 2020
@SurajBhari
Copy link
Copy Markdown
Contributor Author

I don't actually remember the pin point location of them
but I am pretty sure they are free to use.

@SurajBhari
Copy link
Copy Markdown
Contributor Author

Ok I found the source with reverse image search
https://www.freepik.com/free-vector/colorful-mandala-with-floral-shapes_974305.htm
it states Free for personal and commercial purpose with attribution. . Wonder how can we attribute this ?

@SebastiaanZ
Copy link
Copy Markdown
Contributor

In this case, we can add attribution to the readme, in the licenses section, and link back to the original source. Add a comment that this attribution holds for the assets in the diwali folder (see how we do it for lemoji in the readme) and mention that the work is derivative (as opposed to using the asset as-is).

Kronifer
Kronifer previously approved these changes Mar 9, 2021
Copy link
Copy Markdown

@Kronifer Kronifer left a comment

Choose a reason for hiding this comment

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

Everything looks great!

@SurajBhari
Copy link
Copy Markdown
Contributor Author

i think with this
the pr is ready to be merged.
stale.

Copy link
Copy Markdown
Contributor

@lemonsaurus lemonsaurus left a comment

Choose a reason for hiding this comment

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

Gorgeous assets. Attribution looks adequate. Let's merge.

@lemonsaurus lemonsaurus merged commit af438ed into python-discord:master Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants