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

Set technology sorting #365

Closed
wants to merge 3 commits into from
Closed

Set technology sorting #365

wants to merge 3 commits into from

Conversation

usernaimandrey
Copy link
Contributor

Снимок экрана от 2022-09-19 15-22-47

@ashikov ashikov added the enhancement New feature or request label Sep 20, 2022
@grozwalker
Copy link
Contributor

Сильно бросается в глаза то, что здесь решено две задачи в рамках одного ПР, это не очень хорошо, надо разделить на два. Здесь оставить только сортировку по технологиям, а нотификацию в другом. Сделаешь?

@grozwalker
Copy link
Contributor

И момент по самому коду сортировки: все-таки сортировка и вытаскинвание имен - это не совсем репозиторий, ведь нам нужно просто как-то по особенному показать данные -- это презентер какой-нибудь, но точно не репозиторий и это точно не скоуп.

Еще мы уже чисто практическим путем пришли к тому, что ордеры в скоупах не очень идея, потому что потом где-то нужен другой порядок и это начинается боль (но к данному случаю не относится)

@grozwalker
Copy link
Contributor

хм... увидел второй ПР как рах с нотификацией

@usernaimandrey
Copy link
Contributor Author

И момент по самому коду сортировки: все-таки сортировка и вытаскинвание имен - это не совсем репозиторий, ведь нам нужно просто как-то по особенному показать данные -- это презентер какой-нибудь, но точно не репозиторий и это точно не скоуп.

Еще мы уже чисто практическим путем пришли к тому, что ордеры в скоупах не очень идея, потому что потом где-то нужен другой порядок и это начинается боль (но к данному случаю не относится)

Привет! Пр по нотификации увидел что приняли, сортировку тэгов в ближайшее время по фикшу.

@grozwalker
Copy link
Contributor

Добро, ожидаю)

@usernaimandrey
Copy link
Contributor Author

Добро, ожидаю)

Вроде по фиксил, сделал отдельный презентер TagPresenter, за экстендел его в модель Vacancy, и передал данные через контроллер во вью.

@grozwalker
Copy link
Contributor

Но твой ПР все равно содержит нотификацию! Выпили ее

@grozwalker
Copy link
Contributor

А так вроде все отлично!

@usernaimandrey
Copy link
Contributor Author

Но твой ПР все равно содержит нотификацию! Выпили ее

А теперь дошло) А как так вышло, ведь те изменения я делал в отдельной ветке, какую ошибку я допустил, почему изменения с той ветке попали в этот коммит, и как правильно их выпилить(не знаю к своему стыду), просто в таком ключе с гитом первый раз работаю)

@grozwalker
Copy link
Contributor

а если сделать ребейз мастера в эту ветку? И потом зафорспушить? П.с. если что, пиши мне в слак

dependabot bot and others added 3 commits September 30, 2022 15:49
Bumps [terser](https://github.com/terser/terser) from 5.7.2 to 5.14.2.
- [Release notes](https://github.com/terser/terser/releases)
- [Changelog](https://github.com/terser/terser/blob/master/CHANGELOG.md)
- [Commits](https://github.com/terser/terser/commits)

---
updated-dependencies:
- dependency-name: terser
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
@usernaimandrey usernaimandrey closed this by deleting the head repository Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants