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

Partial closure #849

Merged
merged 5 commits into from Nov 8, 2018
Merged

Partial closure #849

merged 5 commits into from Nov 8, 2018

Conversation

0xZick
Copy link
Contributor

@0xZick 0xZick commented Nov 7, 2018

For the correct PR bonus tokens ✨
💔Thank you!

config/mainnet/erc20.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@caffeinum caffeinum left a comment

Choose a reason for hiding this comment

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

убери не относящиеся изменения, пожалуйста

{ id: 2, context: `2. To list your tokens, please send us the link to your peoject, erc20 address, icon, name and ticker (short name) to team@swap.online;`, },
{ id: 3, context: `3. We as well will announce listing of your tokens on our social media, mutial PR will benefit both sides.`, },
{ id: 4, context: `Crypto Pink Sheets: ALL Coins Welcome To SWAP.Online!`},
{ id: 1, context: `1. We can list your token on swap.online ;` },
Copy link
Collaborator

Choose a reason for hiding this comment

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

зачем это тут?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

там двойные ID и проект не запускается даже, тебя это не смущает ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

почему на develop запускается?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

там много что запускается он его просто собирает, а ты попробуй по ходить по страницам, там надо хотя бы E2E написать которые будут просто открывать все станицы и модальные окна отловим кучу багов)

Copy link
Collaborator

Choose a reason for hiding this comment

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

это изменения не для этого пулл-реквеста, их нужно делать в отдельном

Copy link
Contributor Author

@0xZick 0xZick Nov 8, 2018

Choose a reason for hiding this comment

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

нет я так не думаю, в целом да такой подход правильнее, но когда явно не верно сделано

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • в твоих старых коммитах невозможно найти проблему
  • каждый такой "фикс" это потенциально новый баг, и проверять его нужно отдельно
  • никто нигде не пишет, что нужно в один коммит пихать изменения, которые случайно заметил
  • культура коммитов уж точно важнее, чем архитектура приложения

@noxonsu noxonsu merged commit ed010d6 into develop Nov 8, 2018
@0xZick 0xZick deleted the partialClosure branch November 12, 2018 14:05
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

4 participants