Report reason plugin #163
Report reason plugin #163
Conversation
|
Rzeczywiście, teraz widzę, ze problem występuje również u mnie. Rozwiązaniem wydaje się wejście pod
Rzeczywiście, teraz powinno już działać (głupi błąd 😟 )
Niebardzo przychodzi mi teraz inne rozwiązanie do głowy, generalnie chodzi o to żeby zamienić przyciski unflag/flag ze sobą oraz pokazać info o zgłoszeniu. Teoretycznie można kombinować z AJAXem żeby pobrał całą stronę z silnika i tylko podmienił daną zawartość, ewentualnie jakoś jsem podmienić te ikonki, tylko potem może się zrobić mały problem z ustawianiem odpowiednich wartości w
Dziwne, u mnie działa (http://prntscr.com/rbkdg1), mógłbyś jakoś podesłać SS tej sytuacji?
Próbowałem to zrobić, lecz cały konflikt polega na błędzie w CSS od ciemnego theme. Niestety został on zmniejszony i cały CSS jest w całej linijce. Porównywanie tego znak po znaku trochę mnei przerosło :( |
Prawda, po ręcznym wejściu do wtyczek w panelu admina działa. Wydaje mi się jednak, że inne pluginy nie mają takiego problemu, czy źle pamiętam? Jakby udało się to jakoś szybko rozwiązać to byłoby fajnie, jak się nie da i wynika to z Q2A to trudno, szkoda czasu.
Tak, teraz już działa. Niestety pojawił się jednocześnie inny problem: okienko z wyborem powodu zgłoszenia nie zamyka mi się nigdy. Wybieram powód, klikam "wyślij", w devtoolsach widzę że się poprawnie zapisało i zgłoszenie jest widoczne po odświeżeniu, ale tak to ani strona się nie odświeża, ani okienko samo nie znika, przeciętny user nie będzie miał pojęcia czy sie wysłało czy nie.
To nadal zachowuje się tak samo
Z tego co na szybko sprawdziłem bez powodów to przecież tak właśnie działało, nie było przeładowań strony. Dla widoku użytkownika teraz nic się nie zmieniło, nadal widzi tylko liczbę zgłoszeń. Zmieniło się tylko dla administracji, która dodatkowo widzi powody i sama może zgłaszać z powodami, ale to tak naprawdę też można zwrócić Ajaxem (nawet gotowy fragment HTMLa) i podstawić. No ale to też jest powiedzmy rzecz fajnie jakby była, ale nie obowiązkowa.
Rozumiem, ale jakoś ten konflikt i tak będzie trzeba rozwiązać przy mergowaniu. Najlepiej jakby takiego pliku builda nie było w repo, no ale tu dla uproszczenia został wrzucony kiedyś tam. Jak rozumiem potrzebne zmiany są w normalnych plikach (nie tylko wynikowym buildzie)? Wtedy najwyżej można będzie przy rozwiązaniu konfliktu wybrać cokolwiek i później już po połączeniu zbudować właściwą wersję. |
No trudno mi trochę powiedzieć co to powoduje i od czego to zależy
Solved
Solved
Solved
No szczerze... nie wiem. Ten PR ma niecałe dwa lata, kod mógł się zmienić i pewnie stąd te konflikty. Jak dobrze pamiętam to cały styl od powodów zgłoszeń jest w |
|
Ze względu na zmianę wersji PHP z 5.6 na 7.1 (#166) prośba o rebase zmian z mastera na tę gałąź, tu jest sporo rzeczy, więc trzeba mieć pewność że będą działały też na najnowszej. Wydaje mi się, że po aktualizacji nadal wszystko działa. Tylko nie wyświetla nazwy pluginu w panelu admina na zakładce wtyczki - jest jako "wtyczka bez nazwy". Rozwiązaniem jest dodanie pliku metadata.json w katalogu pluginu z danymi jakie są w qa-plugin.php i powinno być ok.
Ok, to zostawmy, myślę że to nie jest straszny problem, szkoda czasu.
Czyli jak rozumiem wszystkie zmiany są w plikach źródłowych SCSS, więc moglibyśmy zmergować wybierając którąkolwiek wersję zbudowaną, a później gdy zmiany w SCSS będą już razem połączone to zrobić po prostu builda z tego wszystkiego. Jednak jeśli będziesz robił rebase całości to i wszystkie nowe zmiany w SCSS powinny się pojawić na tej gałęzi. Wtedy mógłbyś zrobić od razu builda i rozwiązując konflikty można byłoby wziąć tego z Twojej gałęzi, bo byłoby pewne że zawiera wszystko. Poza tym wydaje mi się, że wszystko działa jak należy. Pozostanie przed mergem kwestia przemyślenia jakie mają być te gotowe powody (możemy to obgadać w jakimś większym gronie czy rzucić konkretną propozycja i zebrać uwagi), bo to też jest w kodzie i będzie gotowe. |
| if (in_array($event, $flagEvents, true)) { | ||
| $postId = $params['postid']; | ||
|
|
||
| qa_db_query_sub(' | ||
| DELETE FROM `^flagreasons` | ||
| WHERE userid = # | ||
| AND postid = # | ||
| ', $userId, $postId); | ||
| } |
There was a problem hiding this comment.
to prowinna być osobna prywatna metoda
| if (in_array($event, $flagEvents2, true)) { | ||
| $userLevel = qa_get_logged_in_level(); | ||
|
|
||
| if ($userLevel >= QA_USER_LEVEL_EDITOR) { | ||
| $postId = $params['postid']; | ||
| } | ||
|
|
||
| qa_db_query_sub(' | ||
| DELETE FROM `^flagreasons` | ||
| WHERE postid = # | ||
| ', $postId); | ||
| } | ||
| } |
There was a problem hiding this comment.
to powinna być osobna prywatna metoda
|
|
||
| public function head_script() | ||
| { | ||
| qa_html_theme_base::head_script(); |
There was a problem hiding this comment.
lepszym jest chyba użycie parent::
| $this->output( | ||
| ' | ||
| <script> | ||
| const flagAjaxURL = "' . qa_path('ajaxflagger') . '"; | ||
| const flagQuestionid = ' . $this->content['q_view']['raw']['postid'] . '; | ||
| </script> | ||
| ' | ||
| ); | ||
| $this->output( | ||
| ' | ||
| <script type="text/javascript" src="' . QA_HTML_THEME_LAYER_URLTOROOT . 'script.js"></script> | ||
| <link rel="stylesheet" href="' . QA_HTML_THEME_LAYER_URLTOROOT . 'styles.css"> | ||
| ' | ||
| ); |
There was a problem hiding this comment.
można to połączyć przecież w jeden output
| $q_view['form']['buttons']['flag']['tags'] = | ||
| 'data-postid="' . $q_view['raw']['postid'] . '" data-posttype="q" '; | ||
| } | ||
| qa_html_theme_base::q_view_buttons($q_view); |
|
|
||
| public function process_request($request) | ||
| { | ||
|
|
|
|
||
| $logged = qa_is_logged_in(); | ||
| if (!$logged) { | ||
| exit(); |
There was a problem hiding this comment.
To jest endpoint pod który idzie request od frontu, nie ma więc sensu generować cały front z pomocą qa_prepare_content(), chyba ze chodzi o jakiegos prostego JSONa w stylu {"message": "You must be logged in to perform this action"}
| } | ||
| } | ||
| } elseif ('a' === $postType) { | ||
| $answerId = $postId; |
There was a problem hiding this comment.
sugarcode $postId w takim razie powinien się inaczej nazywać - może $objectId?
| ' | ||
| INSERT INTO `^flagreasons` (`userid`, `postid`, `reasonid`, `notice`) | ||
| VALUES (#, #, #, $) | ||
| ', $userId, $answer['postid'], $reasonId, $notice |
There was a problem hiding this comment.
skad pewnosc ze w answer bedzie postid? A jak nie zwróci nic?
There was a problem hiding this comment.
No zakładam że jeśli jest triggerowany event o jakimś ID to prawdopodobnie jest on w bazie, ale fakt, sprawdzę to.
| @@ -2312,6 +2312,7 @@ input[type="submit"], button { | |||
| background-image: url('images/icons/edit-white.png'); | |||
| } | |||
| .qa-form-light-button-flag { | |||
| cursor: pointer; | |||
There was a problem hiding this comment.
czy nie da się zrobić tych zmian nienadpisując istniejącego qa-styles? Nie wiem - przy pomocy pluginu? :)
| @@ -933,11 +933,11 @@ blockquote p { | |||
| } | |||
There was a problem hiding this comment.
czy da się te zmiany wydzielić do osobnego pliku css, który jest w pluginie?
There was a problem hiding this comment.
Teoretycznie wszystkie zmiany które są potrzebne do działania pluginu są w osobnym pliku, tutaj w większości są tylko zmiany formatowania. Mimo zrobionego checkouta nadal pokazuje jakieś zmiany w tym pliku, nie mam zielonego pojęcia dlaczego 🤔
| const body = document.body; | ||
| const closer = document.querySelector(".close-preview-btn"); | ||
| const sendButton = document.querySelector(".qa-go-flag-send-button"); | ||
| const errorMessage = "Błąd serwera. Proszę spróbować za jakiś czas"; |
There was a problem hiding this comment.
Dla czytelności osobno zgrupowałbym zmienne, które trzymają elementy DOM i osobno zmienne, które trzymają inne dane (jak w tym przypadku komunikaty).
| const commentSubmitButton = document.querySelector('.qa-form-tall-button-comment') | ||
|
|
||
| function showPopup() { | ||
| flagboxPopup.classList.remove("hide"); |
There was a problem hiding this comment.
Czy pokazanie popupa koniecznie wymaga usunięcia jednej klasy i dodanie drugiej? Jeśli popup jest na starcie ukryty, to niech ma najpierw nadaną klasę "hide", a jej usunięcie go pokaże (albo w CSS ustawione display: none, który "zdejmie" nadanie klasy "show"). Nie widzę potrzeby, aby manipulować dwiema klasami w tym przypadku..
| flagboxPopup.classList.add("show"); | ||
| } | ||
| function hidePopup() { | ||
| flagboxPopup.classList.remove("show"); |
There was a problem hiding this comment.
Analogiczna sytuacja jak w funkcji showPopup() - nie ma potrzeby manipulacji dwoma klasami, aby schować popup.
| paragraph.textContent = errorText; | ||
|
|
||
| removeAllChildFromErrorPopup(); | ||
| errorPopup.appendChild(paragraph); |
There was a problem hiding this comment.
Dodanie nowego elementu do DOM lepiej robić po wszystkich jego manipulacjach w danym czasie - przesuń to linię niżej.
| location.reload(); | ||
| }); | ||
|
|
||
| body.addEventListener("click", (event) => { |
There was a problem hiding this comment.
Tak długą funkcję anonimową lepiej wydzielić do osobnej funkcji - będzie się to lepiej czytać.
| if (event.target.matches(".qa-form-light-button-unflag")) { | ||
| location.reload(); | ||
| } | ||
| if (event.target.matches(".qa-form-light-button-clearflags")) { |
There was a problem hiding this comment.
Ten przypadek można dodać jako OR do if wyżej, a nawet wrzucić .qa-form-light-button-clearflags po przecinku do selektora w if w 45 linii.
| showPopup(); | ||
|
|
||
| flagboxPopup.addEventListener("click", () => { | ||
| hidePopup(); |
There was a problem hiding this comment.
Referencję do hidePopup można umieścić bezpośrednio jako drugi parametr do event listenera. Nie ma potrzeby zawijać tego w funkcję anonimową
|
|
||
| closer.addEventListener("click", () => { | ||
| hidePopup(); | ||
| return false; |
There was a problem hiding this comment.
Czy tutaj nie wystarczy wywołać event.preventDefault, uprzednio przekazując event jako parametr?
|
|
||
| sendButton.addEventListener("click", () => { | ||
|
|
||
| const flagReason = document.querySelector("input.qa-spam-reason-radio:checked"); |
There was a problem hiding this comment.
Czy referencja do tego elementu nie może być pobrana na górze skryptu?
| sendButton.addEventListener("click", () => { | ||
|
|
||
| const flagReason = document.querySelector("input.qa-spam-reason-radio:checked"); | ||
| const flagNotice = document.querySelector(".qa-spam-reason-text").value; |
|
|
||
| if (!flagReason) { | ||
| showError(reportReasonEmptyError); | ||
| return false; |
There was a problem hiding this comment.
Czy nie wystarczy wywołać event.preventDefault?
| return false; | ||
| } | ||
|
|
||
| const { postid: postId, posttype: postType, parentid: parentId } = flagButton.dataset; |
There was a problem hiding this comment.
Nie pamiętam jak stare przeglądarki wspieramy, ale destrukturyzacja nie jest obsługiwana w IE11.
There was a problem hiding this comment.
Nie widzę, żeby property parentId było gdzieś używana w kodzie - czy jest sens to rozpakowywać?
| } | ||
|
|
||
| const { postid: postId, posttype: postType, parentid: parentId } = flagButton.dataset; | ||
| const dataArray = {questionid: flagQuestionid, postid: postId, posttype: postType, reasonid: flagReason.value, notice: flagNotice}; |
There was a problem hiding this comment.
Nazwa zmiennej dataArray sugeruje, że jest to tablica, a to przecież obiekt.
| let isError = false; | ||
|
|
||
| if(flagReason) { | ||
| sendButton.value = "Wysyłanie..."; |
There was a problem hiding this comment.
Zmiana tekstu na przycisku i jego blokowanie można zamknąć w osobnej funkcji.
| $.ajax({ | ||
| type: "POST", | ||
| url: flagAjaxURL, | ||
| data: { ajaxdata: sendData }, |
There was a problem hiding this comment.
Czy backend wymaga, żeby przesyłane dane były obiektem zawierającym property ajaxdata? Nie wystarczy tutaj po prostu przypisać zmienną sendData?
There was a problem hiding this comment.
Czy backend wymaga [...]
Tak
| cache: false, | ||
| success: function(data) { | ||
| if(data.error) { | ||
| if (data.error.includes("Zbyt wiele zgłoszeń. Spróbuj ponownie za godzinę")) { |
There was a problem hiding this comment.
Czy nie można bardziej precyzyjnie wykryć, że mamy do czynienia z konkretnym błędem (np. jakiś kod), tylko trzeba sprawdzać tekst, który najpewniej widzi użytkownik?
| if(data.error) { | ||
| if (data.error.includes("Zbyt wiele zgłoszeń. Spróbuj ponownie za godzinę")) { | ||
| showError(tooManyReportError); | ||
| return false; |
There was a problem hiding this comment.
W jakim celu jest to return false? Czy ma jakiś wpływ na działanie metody success w jQuery?
Mariusz08
force-pushed
the
report-reason-plugin
branch
from
0a33d0b
to
da90fb9
Compare
Prawdopodobnie dałoby się to zrobić, ale jestem bardziej backendowcem, i na widok narzędzi budujących od frontu mam gorączkę. Nie dam rady tego zaadoptować do obecnego skryptu, więc chyba musi tak zostać :( |
|
Na początek utwórz nowego brancha z tego, na którym jesteś i na nim zrób W tamtym PR jest obsługa zgłoszeń od strony użytkownika - nie ma implementacji dla Administracji - planowałem to dorobić, ale backendu za bardzo nie było. Skoro jednak Ty coś rzeźbisz w tym temacie, to jest (kolejna) szansa na dowiezienie tego. :) |
Cześć,
To przerobiony plugin od q2apro i przystosowany pod nasze realia, wydaje się działać dobrze.
Był już jeden PR od tego pluginu ale zrobił się strasznie długi przez moje nieudolne próby naprawiania go, więc tworzę nowy żeby nie robić syfu i każdy mógł się połapać.
@CodersCommunity/forum