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

Вынести в d3.content определение местоположения пользователя (главная, пост, инбокс, ...) #31

Open
Aivean opened this issue Nov 26, 2012 · 6 comments

Comments

@Aivean
Copy link
Collaborator

Aivean commented Nov 26, 2012

Пример из модуля hide.inbox.users:

    var insidePost = ( document.location.href.indexOf('comments') > -1 );
    var insideInbox = ( document.location.href.indexOf('inbox') > -1 );
@crea7or
Copy link
Collaborator

crea7or commented Nov 28, 2012

Это потому, что я не помню как оно в d3 content(или где-то там) называется. А найти мне тяжело, я крутой js код очень трудно читаю. А там кое что уже есть.
надо по хорошему так:

insidePost, insideInbox, postsList, inboxList, mainPage (не подсайтовая главная)

@Aivean
Copy link
Collaborator Author

Aivean commented Nov 28, 2012

Да, есть d3.page

d3.page=
{
    inbox: document.location.pathname.substr(0,10)=="/my/inbox/",
    onlyNew: (document.location.href.indexOf('#new') > -1),
    user: (window.location.pathname.indexOf("/user/")>=0) || (window.location.pathname.indexOf("/users/")>=0)
};

Возможно, я сейчас напишу очевидные вещи, но все же.
Лучше выносить подобные вещи в одно общее место, потому что:

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

Кстати, предложенный вариант:
insidePost, insideInbox, postsList, inboxList, mainPage
не совсем правильный с точки зрения дизайна, поскольку есть взаимоисключающие состояния. Например, inboxList и mainPage никода одновременно не будет true.

Лучше сделать, например, так: hasPosts, hasComments и currentPage={main,inbox,my,...}

@crea7or
Copy link
Collaborator

crea7or commented Nov 28, 2012

можно и так, просто я сужу со стороны определения местоположения - логичнее звучит. если бы в js были дефайны, было бы ещё проще типа: currentLocation и там все варианты.

@Aivean
Copy link
Collaborator Author

Aivean commented Nov 28, 2012

По примерно этим же причинам надо будет вынести в общее место многие другие часто используемые вещи, такие как:

  • в классах Post и Comment добавить больше функциональности по определению частей. Сейчас, например, можно получить username, а нужно еще контейнер с username. Сейчас в двух модулях это дело захардкоджено. И т.д.
  • Вынести куда-то ссылки на общие блоки. Например, div со всеми комментами.
  • Вынести в проперти имена частоиспользуемых классов. Частично это уже сделано, например, в классе Post пропертя contentClass. Надо расширять.

@Aivean
Copy link
Collaborator Author

Aivean commented Nov 28, 2012

если бы в js были дефайны, было бы ещё проще типа: currentLocation и там все варианты.

я это и пытался написать, поле currentPage, которая может принимать значение одной из констант main, inbox, ...

@crea7or
Copy link
Collaborator

crea7or commented Nov 28, 2012

константы текстом? ну можно и так.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants