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

Custom emoji mapping #2198

Merged
merged 1 commit into from
Nov 15, 2018
Merged

Custom emoji mapping #2198

merged 1 commit into from
Nov 15, 2018

Conversation

Pomax
Copy link
Contributor

@Pomax Pomax commented Nov 15, 2018

Closes #2196, as long as we can make sure the minimum and maximum values are acceptable =)

@cadecairos cadecairos temporarily deployed to foundation-mofostaging-pr-2198 November 15, 2018 19:33 Inactive
@@ -21,7 +21,7 @@
"optimize:png": "find source/images -type f -name '*.png' -print0 | xargs -0 -n 1 -P 6 optipng",
"optimize": "run-p optimize:**",
"server": "pipenv run python network-api/manage.py runserver",
"start": "npm i && npm run build-uncompressed && run-p server watch:**",
"start": "npm i && run-p build-uncompressed server watch:**",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mostly a dev Quality of Life change - this starts the server way earlier, so you can just reload the tab once webpack finishes the build, instead of getting a "server not found" for like... a minute

Copy link

@kristinashu kristinashu left a comment

Choose a reason for hiding this comment

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

Yes! So nice to see "not creepy!"

// creepy by default.
//
// Note: this is a cosmetic value for scroll only.
const MAXIMUM_CREEPINESS_RATING = 80;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are MINIMUM_HAPPINESS_RATING and MAXIMUM_CREEPINESS_RATING both fixed values unrelated to EMOJI_FRAME_HEIGHT and SPRITE_FRAME_COUNT? If there's any relationship between them I think it's worth adding some inline comments in case we decide to change the values for frame height and frame count in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is zero relation between the two sets of numbers.

Copy link
Collaborator

@mmmavis mmmavis left a comment

Choose a reason for hiding this comment

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

I left one comment but code looks good to me! ✨

@Pomax Pomax merged commit ecd3051 into master Nov 15, 2018
@Pomax Pomax deleted the custom-emoji-mapping branch November 15, 2018 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants