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.

Sign up for GitHub

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

Jump to bottom

feat: mark ukrainian links differently #11

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: mark ukrainian links differently #11

wants to merge 3 commits into from

Conversation

undead404
Copy link
Contributor

Mark Ukrainian & other links with some distinct class names.

@undead404 undead404 added the enhancement New feature or request label Mar 11, 2023
@undead404 undead404 requested a review from AdriandeCita March 11, 2023 05:57
@undead404 undead404 self-assigned this Mar 11, 2023
@undead404 undead404 mentioned this pull request Mar 11, 2023
Open
AdriandeCita
AdriandeCita requested changes Mar 11, 2023
View reviewed changes
Copy link
Member

@AdriandeCita AdriandeCita left a comment

Choose a reason for hiding this comment

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

Дякую, прикольна фіча. Я досі не певен в її практичності, але як ідея — гарно.

Пара зауважень стосовно класів.

  • ми можемо зробити цю логіку більш загальною? У нас насправді частка uk виникає не зі стелі, а з параметрів раннера
    image

Код процесора (принаймні, теоретично), можна швидко адаптувати під будь-яку локаль перекладу. А функція isUrlUkrainian порушує цей принцип. Тоді сам клас wd-link__ukrainian краще буде представити якось на кшталт wd-link__target-locale, а вже платформа буде вирішувати, який прапорець (чи не прапорець) там показувати.
Регулярки для урлів теж краще передавати зовні, щоб на кожну нову урлу не доводилося випускати нову версію бібліотеки.

@undead404
Copy link
Contributor Author

Хіба що перейменувати isUrlUkrainian на isUrlInLocalLanguage. Код цієї функції ж треба буде на кожну мову писати окремо.

@undead404
Copy link
Contributor Author

Тоді можна буде переписати тіло цієї функції – і контент-процесор буде готовий

@AdriandeCita
Copy link
Member

чому окремо, якщо ти просто використовуєш масив регулярок? Тим паче, якщо їх передавати зовні?
Одна функція, просто перебирання передані регулярки - і все? Чи я тут упускаю якісь підводні граблі?)

@undead404
Copy link
Contributor Author

Передавати зовні регулярки? Звідки? Щось ти дуже ускладнюєш

@undead404 @AdriandeCita
Apply suggestions from code review
5a4f9e2 
Co-authored-by: Mykola Myslovskyi <[email protected]>
@AdriandeCita
Copy link
Member

ага, карту редиректів ми передаємо, а масив регулярок передати не можемо?)

@AdriandeCita
Copy link
Member

AdriandeCita commented Mar 11, 2023 •
edited
Loading

ох, дідько, нафіга я той ПР змержив, якщо я передаю редиректи зовні... По ходу треба буде його сьогодні ревертнути, поки воно в СІ не потрапило х)
Поправив

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AdriandeCita AdriandeCita AdriandeCita requested changes

Assignees

@undead404 undead404

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@undead404 @AdriandeCita @viperehonchuk