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

Add support for skip gif flag #6

Merged
merged 8 commits into from May 19, 2020

Conversation

lwaddicor
Copy link
Contributor

This adds the ability to disable the posting of gifs in the comments.
Includes the addition of a flag, tests and documentation updates

This adds the ability to disable the posting of gifs in the comments.
Includes the addition of a flag, tests and documentation updates
@rheaditi
Copy link
Contributor

Hey, @lwaddicor!

thanks for this PR! Looks great!

Was wondering, would it be useful if we were to give the option to customize the comment body, instead of specific options to disable gifs?

@rheaditi
Copy link
Contributor

@all-contributors please add @lwaddicor for code

@allcontributors
Copy link
Contributor

@rheaditi

I've put up a pull request to add @lwaddicor! 🎉

src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@rajanand02 rajanand02 left a comment

Choose a reason for hiding this comment

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

@rheaditi @lwaddicor The PR looks good to me. Just a minor nitpick comment. Please fix the conflict in lib/index.js then we are good to merge this PR. @rheaditi Please test the comments in one the repo, we had faced issues in the final render of the comments in the past.

@lwaddicor
Copy link
Contributor Author

Hey, thanks for the reviews, i will try to get this conflict fixed tommorow.

@rheaditi yeah a custom comment comment ability would be a pretty cool feature to add

@rheaditi
Copy link
Contributor

@lwaddicor @rajanand02 I'm thinking.. let's remove the gifs by default? It was cool at first but it consumes a lot of space in a PRs comment section and consumes bandwidth 😅

@rajanand02
Copy link
Contributor

rajanand02 commented Apr 23, 2020

I'm thinking.. let's remove the gifs by default? It was cool at first but it consumes a lot of space in a PRs comment section and consumes bandwidth

@rheaditi A small cool/fun element won't hurt 😉 When someone trying the tool for the first time they would definitely love the gif but I agree that It can get annoying and also occupying too much space in the conversation section. Since @lwaddicor added an option to disable it I guess it should be fine.

@lwaddicor
Copy link
Contributor Author

@lwaddicor @rajanand02 I'm thinking.. let's remove the gifs by default? It was cool at first but it consumes a lot of space in a PRs comment section and consumes bandwidth

I'm easy either way, as long as you can disable it I dont see the harm :)

@rheaditi
Copy link
Contributor

rheaditi commented Apr 27, 2020

@lwaddicor the lib/index file was deleted in your PR.. this will make the action fail.. please add it back and commit the built file 🙈

@lwaddicor
Copy link
Contributor Author

@lwaddicor the lib/index file was deleted in your PR.. this will make the action fail.. please add it back and commit the built file

@rheaditi my bad, rebuilt it and re-added it

@rheaditi rheaditi merged commit 93613c7 into ClearTax:master May 19, 2020
@rheaditi
Copy link
Contributor

@all-contributors please add @lwaddicor for code

@allcontributors
Copy link
Contributor

@rheaditi

I've put up a pull request to add @lwaddicor! 🎉

rheaditi added a commit that referenced this pull request May 20, 2020
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.

None yet

3 participants