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

Gulpfile Edits #759

Merged
merged 8 commits into from
Jul 18, 2021
Merged

Gulpfile Edits #759

merged 8 commits into from
Jul 18, 2021

Conversation

roythearsonist
Copy link
Contributor

Makes Gulpfile look better

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.

Overall, the suggested changes are good and I appreciate them. However, there are a few that don't fit the repo's style, such arrow callback with one argument using parentheses or dangling commas in objects.

If you can address those issues, the PR is good to go. Do take a look at the file to make sure everything is gone though, I might have miss some of them.

gulpfile.js Outdated Show resolved Hide resolved
gulpfile.js Outdated Show resolved Hide resolved
gulpfile.js Outdated Show resolved Hide resolved
gulpfile.js Outdated Show resolved Hide resolved
gulpfile.js Outdated Show resolved Hide resolved
gulpfile.js Outdated Show resolved Hide resolved
gulpfile.js Outdated Show resolved Hide resolved
gulpfile.js Outdated Show resolved Hide resolved
gulpfile.js Outdated Show resolved Hide resolved
gulpfile.js Outdated Show resolved Hide resolved
@Thomas-Boi
Copy link
Member

For future references, please avoid making PRs that contains only stylistic changes that don't bring a lot of benefits to the repo. If you are refactoring an absolute mess of a file, that'll be greatly appreciated. However, for files like the gulpfile that don't get worked on often, this is unnecessary.

I know that many of the files are not 100% up to industry gold standards. However, they do meet many of these stylistic standards (readable code, properly commented) and they work. Furthermore, this is a hobby. I don't want to force everyone to follow every small stylistic rules, hence the lack of linters in the project. Instead, if you can dedicate that time to features, that'll be much more appreciated.

@roythearsonist
Copy link
Contributor Author

Alright. Although to be honest, that's kinda all I'm good at. But it is what it is

roythearsonist and others added 4 commits July 17, 2021 22:24
Co-authored-by: Thomas Bui <43018778+Thomas-Boi@users.noreply.github.com>
Co-authored-by: Thomas Bui <43018778+Thomas-Boi@users.noreply.github.com>
Co-authored-by: Thomas Bui <43018778+Thomas-Boi@users.noreply.github.com>
Co-authored-by: Thomas Bui <43018778+Thomas-Boi@users.noreply.github.com>
@Thomas-Boi
Copy link
Member

Alright. Although to be honest, that's kinda all I'm good at. But it is what it is

You can still help out with feature request and icon reviews. Those are easy to pick up and we can always use a hand with them.

Also, thanks for fixing the issues but there are a few left. Just a tip: you can fix them quickly on the website by clicking on "Add suggestion to batch" or "Commit Suggestion"
image

This will let GitHub commit my changes in the PR without the need of you relooking for the suggestions.

roythearsonist and others added 2 commits July 17, 2021 22:44
Co-authored-by: Thomas Bui <43018778+Thomas-Boi@users.noreply.github.com>
Co-authored-by: Thomas Bui <43018778+Thomas-Boi@users.noreply.github.com>
@roythearsonist
Copy link
Contributor Author

Just a tip: you can fix them quickly on the website by clicking on "Add suggestion to batch" or "Commit Suggestion"

Thanks, I never knew that

@Thomas-Boi
Copy link
Member

Good work 👍. Everything looks good now.

Remember, there are many ways to contribute. Just because we don't accept some of them doesn't mean that we don't accept everything. If you want, you can also open an Issue or message us on Discord before you want to do something. That way, you don't have to lose time on your PRs.

@Thomas-Boi Thomas-Boi merged commit f37cec1 into devicons:develop Jul 18, 2021
@roythearsonist
Copy link
Contributor Author

you can also open an Issue or message us on Discord

Can I get the link

@roythearsonist roythearsonist deleted the gulpfile branch July 18, 2021 05:42
@Thomas-Boi
Copy link
Member

@theblobscp here you go: https://github.com/devicons/devicon#discord-server

@Panquesito7
Copy link
Member

you can also open an Issue or message us on Discord

Can I get the link

Here's the link invitation to our Discord server: https://discord.gg/hScy8KWACQ

@roythearsonist
Copy link
Contributor Author

Thanks to you both

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.

3 participants