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

Added list of outdated events in events module #1043

Closed
wants to merge 1 commit into from

Conversation

nicraMarcin
Copy link
Contributor

No description provided.

@nicraMarcin nicraMarcin force-pushed the oldEvents branch 5 times, most recently from 67ba826 to 7572b0b Compare January 4, 2017 20:26
@interduo
Copy link
Collaborator

interduo commented Jan 5, 2017

Interesująca zmiana. Jak za pomocą gita ją zaaplikować do siebie?

@nicraMarcin
Copy link
Contributor Author

fork i merge.
ale to jest kilka linijek, można przekopiować

@kosaa
Copy link
Contributor

kosaa commented Jan 5, 2017

lepiej poczekac na merge bo beda konflikty podczas pulla

@interduo
Copy link
Collaborator

interduo commented Jan 5, 2017

Przed pullem robię git stash jeśli robię zmiany. Chciałem testowo to commitnąć. Jak to z poziomu gita zrobić?

@kosaa
Copy link
Contributor

kosaa commented Jan 5, 2017

a to wtedy reset czy stash i jest ok, tak tylko ostrzegam bo moga byc czasami problemy

@chilek
Copy link
Owner

chilek commented Jan 5, 2017

Merge-a raczej nie będzie, bo po przejrzeniu doszedłem do wniosku, że za dużo duplikacji kodu...

@interduo
Copy link
Collaborator

interduo commented Jan 5, 2017

Zmiana fajna, jak proponujesz to przepisać?

@nicraMarcin
Copy link
Contributor Author

nicraMarcin commented Jan 5, 2017

Tomku, duplikacji musi trochę być, bo:

  1. pobieramy nie zamknięte wstecz do dnia "dzisiejszego"
  2. musimy pobrać dane do zdarzeń

by nie było duplikacji trzeba by przerobić obecną funkcję GetEvetnList by w warunkach jej podawać kolejne atrybuty i w ciele je obsługiwać.

pobierając eventlist pobieramy zakres od do, tu w oldEventlist pobieramy nie zamknięte do dzisiaj. Więc są osobne funkcje, które pobierają to samo lecz z innego zakresu.

@nicraMarcin
Copy link
Contributor Author

Zrobiłem małą reorganizację i zmniejszyłem liczbę linii kodu php

@nicraMarcin
Copy link
Contributor Author

nicraMarcin commented Jan 6, 2017

@interduo jak Ci się podobają zmiany to możesz pobrać sobie diffa z każdego kommita osobno
https://patch-diff.githubusercontent.com/raw/lmsgit/lms/pull/1043.diff
https://patch-diff.githubusercontent.com/raw/lmsgit/lms/pull/1043.patch

@nicraMarcin
Copy link
Contributor Author

Tomku, co Twoim zdaniem trzeba poprawić byś mergował?
Jeśli Twoim zdaniem zmiana nie potrzebna to odrzuć.

@interduo
Copy link
Collaborator

@chilek to by się przydało i nam, co trzeba zrobić byś zaakceptował merge?
Chcę pomóc w przepisaniu tego. Może stworzyć funkcję GetOldEvents i używać jej w tym okienku?

@chilek
Copy link
Owner

chilek commented Sep 11, 2017

Przeczytajcie moje komentarze - za dużo duplikacji kodu. Żeby opisać co trzeba zrobić to chyba lepiej usiąść i samemu to napisać...

@interduo
Copy link
Collaborator

Twój komentarz jest bardzo ogólny ciężko więc dojść co masz na myśli, commit niby nie jest duży.
Możesz uchylić rąbka tajemnicy czy chodzi o backend czy frontend albo wskazać linie?

@chilek
Copy link
Owner

chilek commented Sep 11, 2017

To:
https://github.com/lmsgit/lms/pull/1043/files#diff-9f31fced3035521aa58fa261f05d4f3a
czyli frontend.
Można byłoby wydzielić część html wspólną dla bieżących i starych zdarzeń do oddzielnego pliku podszablonu i parametrycznie przekazywać co ma być wyświetlone.

@interduo interduo mentioned this pull request Jun 21, 2018
@interduo
Copy link
Collaborator

To jest do zamknięcia.

@chilek chilek closed this Jul 30, 2018
@nicraMarcin nicraMarcin deleted the oldEvents branch July 9, 2019 13:20
chilek added a commit that referenced this pull request Feb 10, 2020
chilek added a commit that referenced this pull request Feb 14, 2020
interduo pushed a commit to interduo/lms that referenced this pull request May 26, 2020
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.

4 participants