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

RSS Виртуальная клавиатура #1

Open
wants to merge 11 commits into
base: Review
Choose a base branch
from

Conversation

tensegrity666
Copy link
Owner

@tensegrity666 tensegrity666 commented Apr 6, 2020

  • нажатие на кнопку на физической клавиатуре подсвечивает кнопку на виртуальной (проверять следует нажатие цифр, букв, знаков препинания, backspace, del (если она присутствует), enter, shift, alt, ctrl, tab, caps lock, space, стрелки вниз-вверх-влево-вправо) -10 (стрелки сильно портят дизайн, поэтому отсутствуют. Часть символов не реализована)
  • реализована анимация нажатия на кнопку: -5 (анимация шифтов, альтов и контролов дублируется)

Итого: -15

@tensegrity666 tensegrity666 changed the title fix: fix stylelint problems RSS Виртуальная клавиатура Apr 6, 2020
@966647
Copy link
Collaborator

966647 commented Apr 11, 2020

  1. По кодстайлу:
    Названия функций должны в полной мере отражать что делает функция и должны именоваться в единой манере, например в императиве в camelCase (т.е. например функции для инициализации: initKeyboard, initCapslockProcessing, initState)
    Константные значения надо вынести в константы и можно также в зависимости от смысла вынести в отдельный модуль constants либо определить наверху своего модуля

  2. Использовать классы для хранения состояния приложения не рекомендуется, а уж использовать одни и те же классы и для стилей и для хранения состояния вообще нельзя. Классы в html - для стилей. В крайнем случае для хранения состояния в html можно использовать data-* атрибуты (dataset), но это тоже не очень удобно, лучше состояние реализовывать вообще отдельно, а уже исходя из состояния строить представление.

  3. Использовать в разных частях приложения localStorage для хранения состояния приложения также не очень правильная практика. Тем более в таком виде в каком это реализовано тут - прямой доступ по строке к свойствам. Эта браузерная фича для сохранения данных и для этих целей его и надо использовать - т.е. просто при изменении состояния приложения сохранять его копию в localStorage и делать это не по общим названиям как buffer и т.д., а по определенному ключу, который точно не будет конфликтовать с другими входами в ls на странице, скажем "rss_virtualKeyboard_lskey". В идеале всю работу с localStorage лучше вынести в отдельный модуль

  4. В целом в коде всё намешано - и логика клавиатуры и браузерные фичи как localStorage и пользовательские ивенты как нажатие клавиш и для читабельности/удобства изменений и т.д. лучше бы отделить модель данных с логикой от представления. Для этого как вариант можно использовать стандартную MVC архитектуру с толстой моделью. (С вопросами по реализации можно в любое время писать мне в скайп).
    Если коротко, то модель данных со всем состоянием - нажаты ли клавиши капслок, шифт, альт, контрл, текущее введённое значение, текущий язык, логика клавиатуры, а именно логика изменения текущего введенного значения в зависимости от других нажатых в текущее время клавиш и интерфейс(методы) для работы с моделью. Ну и дальше пляшешь от этой модели - согласно текущей модели делаешь представление и при пользовательских действиях меняешь модель, сохраняешь измененную модель в localStorage и обновляешь представление.

  5. В части представления работа с html dom как со строкой будет быстрее и гибче, т.е. строишь дом дерево и вставляешь его или через innerHTML, или парсишь через DOMParser. Далее через селекторы достаешь ссылки на все нужные части и с ними работаешь, обновляешь

  6. Не учтены альтернативные вводы для клавиш с цифрами, нет вообще курсора, нет различий в нажатии левого и правого шифта, статичный размер, с точки зрения UI очень много пространства между клавишами между тем как сами клавиши маленькие

@tensegrity666
Copy link
Owner Author

6\. Не учтены альтернативные вводы для клавиш с цифрами, нет вообще курсора, нет различий в нажатии левого и правого шифта, статичный размер, с точки зрения UI очень много пространства между клавишами между тем как сами клавиши маленькие

Пофиксил немного UI (размер клавиш, вернул курсор, резиновый размер).

Остальные замечания учту для следующих проектов, здесь проще удалить весь код и переписать с нуля.

@966647
Copy link
Collaborator

966647 commented Apr 14, 2020

Я больше оценивал кодстайл и архитектуру, Сергей оценил функциональную часть по клаве и уже замечания от него влияющие на оценку:

  1. Минимальный набор, второй пункт реализован частично: левые и правые Alt, Ctrl и Shift - это всё-таки разные кнопки, но подсвечиваются попарно. Стрелки всё-таки надо было сделать, как в реальной клавиатуре - тем более, что их проверка есть в критерии. При английской раскладке в системе буквы не подсвечиваются. -5баллов
  2. Стандартный набор: первый пункт - проверить переключение раскладки визуально не представляется возможным. При нажатии сочетания клавиш для смены языка в поле пишет Meta. Также вообще не работает язык при вводе с реальной клавы - язык на виртуальной клаве стоит английский, а с реальной клавы печатаются русские буквы. Также выбранный язык не сохраняется (этого не было и ранее когда переключение работало). 0 баллов
  3. Техтребования, третий пункт - PR оформлен плохо, скорее чисто формально по минимуму. -3 балла

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

@tensegrity666
Copy link
Owner Author

  • Стандартный набор: первый пункт - проверить переключение раскладки визуально не представляется возможным. При нажатии сочетания клавиш для смены языка в поле пишет Meta.

Переключение языка в текущей реализации осуществляется клавишей Win (комментарий об этом написан под клавиатурой).
В Линуксе переключение сочетанием клавиш не реализовано (но в задании не требуется кроссплатформенность).
Мету уберу в ближайшем фиксе, это событие попадает в инпут только в Линуксе

  • Техтребования, третий пункт - PR оформлен плохо, скорее чисто формально по минимуму. -3 балл

Добавляю самопроверку.

По части багов фиксы планируются

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.

2 participants