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

improvement: fontawesome, next part of icons #1418

Closed
wants to merge 1 commit into from

Conversation

interduo
Copy link
Collaborator

@interduo interduo commented Oct 8, 2018

No description provided.

@interduo
Copy link
Collaborator Author

interduo commented Oct 9, 2018

Puścisz ikonki?

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

Jest kolizja, bo równolegle przeniosłem wszystko na:

{button ...}

Obeszło się bez robienia oddzielnych elementów button i link. Po prostu w button dałem dodatkowy type o wartości link.

@interduo
Copy link
Collaborator Author

interduo commented Oct 9, 2018

To nie było równolegle :P Mój PR był wcześniej.

git merge tak wygląda czasami :D
https://www.youtube.com/watch?v=H81_ED8S-4w

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

Spróbuj w swoich zmianach od razu przyciski (tam gdzie używasz) przenieść na nową funkcję smarty {button}.

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

Przejrzałem te ostatnie zmiany - kierunek raczej będzie nie ten, tzn. nie wiem czy sensowne jest pakowanie wszystkie w klasy lms-ui-icon-content tylko po to, żeby uzyskać odstęp. Raczej takie coś wypadałoby wbudować w bloki {box_...} i {tab_...}.

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

Jeśli coś jest bardzo nagminne nie warto szpikować dodatkowym kodem html.

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

Dlatego też proponuję na razie z ikonkami nie ruszać szablonów, które są zrobione na tabelkach, bo lepiej wypracować jeden/kilka mechanizmów na mniejszej liczbie tabel niż potem wszystko w kółko modyfikować.

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

W związku z tym proponuję ograniczyć się ze zmianami ikon tylko do przeniesionych na nowy layout szablonów. Mam na myśli te ikonki przy każdym polu.

@interduo
Copy link
Collaborator Author

interduo commented Oct 9, 2018

Jakoś specjalnie buttonów nie tykałem bo wiedziałem że kod się zmieni. Przejrzę je wszystkie ale dopiero pojutrze. Zrób merge&squash, żeby znów się to nie rozjechało.

@interduo
Copy link
Collaborator Author

interduo commented Oct 9, 2018

Przejrzałem te ostatnie zmiany - kierunek raczej będzie nie ten, tzn. nie wiem czy sensowne jest pakowanie wszystkie w klasy lms-ui-icon-content tylko po to, żeby uzyskać odstęp. Raczej takie coś wypadałoby wbudować w bloki {box_...} i {tab_...}.

Klasę lms-ui-content łatwo wykasować. Gdy przeniesiemy definicję ikon na nowych kod {icon} to szybciej zmienimy kod bo dodałem sporo klas do style.less. I łatwiej będzie to z automatu zmienić.

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

Klasę lms-ui-content łatwo wykasować. Gdy przeniesiemy definicję ikon na nowych kod {icon} to szybciej
zmienimy kod bo dodałem sporo klas do style.less. I łatwiej będzie to z automatu zmienić.

Wątpię - zrobi się tylko większy bałagan.

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

Co tam powinno być zamiast 6134? :)

@interduo
Copy link
Collaborator Author

interduo commented Oct 9, 2018

https://github.com/chilek/lms-plus/blob/master/templates/default/rt/rtticketeventlist.html#L14-L22

Poprawiłem to. Nie wyłapałem w webeditorze przy przeglądaniu PR, dzięki.

@interduo
Copy link
Collaborator Author

interduo commented Oct 9, 2018

Część ikon jest w <IMG SRC dużo więcej w <i class. Chcę doprowadzić do sytuacji gdzie będą wszystkie z fontawesome.

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

Część ikon jest w <IMG SRC dużo więcej w <i class. Chcę doprowadzić do sytuacji gdzie będą wszystkie z fontawesome.

Widziałem, że przy okazji wiele rzeczy zmieniasz, a nie tędy droga - bałagan murowany.

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

Zauważyłem, że przy okazji przestało w którymś momencie działać tworzenie zdarzeń terminarza od razu powiązanych ze zgłoszeniem :(

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

Sugeruję nie spieszyć się ze zmianami, bo to potem na jakość odbija się mocno.

@interduo
Copy link
Collaborator Author

interduo commented Oct 9, 2018

Zauważyłem, że przy okazji przestało w którymś momencie działać tworzenie zdarzeń terminarza od razu powiązanych ze zgłoszeniem :(

Zrobiłem test - https://demo.lms.org.pl/?m=eventinfo&id=83. Działa

@interduo
Copy link
Collaborator Author

interduo commented Oct 9, 2018

Ale fakt, że jeśli w _GET jest ticketid określony checkbox nie powinien się wyświetlać.

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

  1. https://demo.lms.org.pl/?m=rtticketview&id=29
  2. Dodaj nowe zdarzenie z tego poziomu powiązane z przeglądanym zgłoszeniem.
    Przed chwilą sprawdziłem - nie działa.

Ogólna uwaga - skoro już robimy powtarzające się elementy zbudowane z kilku horyzontalnie położonych buttonów to też już to mamy (albo prawie mamy) w smarty.

@interduo
Copy link
Collaborator Author

interduo commented Oct 9, 2018

Widziałem, że przy okazji wiele rzeczy zmieniasz, a nie tędy droga - bałagan murowany.

Nie. W tym PR tylko ikonki. Jedynie kilka ikon merytorycznie podmieniłem po konsultacji na inne.

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

A całkowite reformatowania pliku? :)

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

Co prawda PHP Storm daje CTRL+ALF+F lub CTRL+ALT+SHIFT+F.

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

A zmiany linków na przyciski?
Kto potem będzie to porządkował? :)

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

Może dla przykładu zróbmy jeden szablon wspólnie od początku do końca i zobaczymy czy jeszcze w ogóle czegoś brakuje ze strony smarty?

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

Btw. Commitem https://github.com/chilek/lms-plus/commit/70be37dda22b315fc1ac8d887e05bddb0524ad1a
przywróciłem dodawanie nowych zdarzeń do istniejących zgłoszeń.
Moje zmiany layoutu i dodatkowe cleanupy spowodowały rozjazd ;-)

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

Wydaje mi się, że niewiele brakuje od strony smarty, żeby móc zmieniać szablony html, na szablony używające elementów smarty. Są zaimplementowane:
{box_....} - dla całych boksów z danymi - pewnie będzie docelowo dobre dla całych tabel z listami rekordów i dla boksów z danymi typu *info, *edit, *add.
{tab_....} - dla zakładek, gdy wyświetlamy informacje o którymś rekordzie w {box_...}
{button} - dla przycisków typu <a> i <button>

@chilek
Copy link
Owner

chilek commented Oct 9, 2018

Dla zakładek {box_...} fajnie byłoby pójść za ciosem i wszystkie przerobić w CORE. Tym bardziej, że procent zaawansowania tego przedsięwzięcia jest wysoki - zaadaptowane są już prawie wszystkie zakładki klienta i komputera.

@interduo
Copy link
Collaborator Author

interduo commented Oct 9, 2018

https://demo.lms.org.pl/?m=eventedit&id=84 - zazanaczona domyślnie opcja "przypisz do zgłoszenia" ale wartość jej pustaw - tak być nie powinno ponieważ zdarzenie nie jest nigdzie przypisane.

@interduo
Copy link
Collaborator Author

interduo commented Dec 10, 2018

@chilek chciałem dokończyć ten PR,

Było:
<img src="img/account.gif" alt="">

Proponowałem:
<i class="lms-ui-icon-content lms-ui-icon-login"></i>

Jak według Ciebie powinien być wprowadzony ten kod z obrazkami daj proszę przykład na przykładzie tego fragmentu.

@chilek
Copy link
Owner

chilek commented Dec 12, 2018

Mi się jedynie nie podoba: lms-ui-icon-content, bo wszędzie trzeba to wstawiać, a po co?

@interduo
Copy link
Collaborator Author

Jak zrobić inaczej odstępy?

@chilek
Copy link
Owner

chilek commented Dec 12, 2018

Chodzi o odstępy między ikonkami i etykietami tekstowymi? Moim zdaniem całość powinna trafiać do jakiegoś elementu html, który to opakowuje.

@interduo
Copy link
Collaborator Author

Tak.
To w takim razie wywalę tą klasę a potem tam gdzie odstępy są złe opakujemy to w coś.

@chilek
Copy link
Owner

chilek commented Dec 13, 2018

W sumie niektóre elementy są już opakowane w nadrzędny element TD. Można byłoby dla takiego elementu wprowadzić tymczasowo jakąś klasę CSS. Kiedyś pewnie TD wylecą, ale kiedy - daleka przyszłość pewnie.

@interduo
Copy link
Collaborator Author

Ok - usunąłem lms-ui-icon-content z kodu.

Pokaż proszę przykład jak opisać elementy TD w CSS tam gdzie znajdują się ikonki tak, żeby były wyśrodkowane względem siebie. Mi się coś rozjeżdza.

@chilek
Copy link
Owner

chilek commented Dec 17, 2018

Pokaż proszę przykład jak opisać elementy TD w CSS tam gdzie znajdują się ikonki tak, żeby były wyśrodkowane względem siebie. Mi się coś rozjeżdza.

  <TD class="lms-ui-normal-column">
      <i ...></i><span class="bold">Label</span>
  </TD>
  .lmsbox .lms-ui-normal-column > :not(:last-child) {
     margin-right: 0.2em;
  }

Nie testowałem tego powyżej, ale idea została przestawiona. Może warto byłoby jakoś uogólnić to dla table lmsbox* tak, żeby od razu w tabeli td dowolny się łapał na te definicje css?

@interduo
Copy link
Collaborator Author

A jak z wyśrodkowaniem w pionie?

@chilek
Copy link
Owner

chilek commented Dec 17, 2018

A jak z wyśrodkowaniem w pionie?

Tzn. co konkretnie chcesz uzyskać?

@interduo
Copy link
Collaborator Author

Wyśrodkowanie w pionie ikonki zawartej w znaczniuk <i ...></i> wobec tekstu Label.

@chilek
Copy link
Owner

chilek commented Dec 17, 2018

.lmsbox .lms-ui-normal-column {
  vertical-align: middle;
}

Utwórz może w jsfiddle przykład i mi udostępnić - tam ustalimy co i jak.

@interduo
Copy link
Collaborator Author

interduo commented Dec 17, 2018

git checkout interduo-lms/663;

Przykładem może być plik rtticketinfobox.html

@chilek
Copy link
Owner

chilek commented Dec 17, 2018

@interduo
Copy link
Collaborator Author

Widzisz moje zmiany na jsfiddle?

@interduo
Copy link
Collaborator Author

interduo commented Dec 17, 2018

Taki podałeś przykład trochę nie z życia - ikonki są różnych szerokości a jednak przydałoby się żeby:

  • ikonki były nad sobą i wyśrodkowane w pionie względem środka siebie
  • labelki były nad sobą i zaczynały się wszystkie "jak od linijki"
    Nie lubię się babrać we frontendzie bleh.

Jeśli nie widzisz moich zmian to podmieniłem kod na:

<table class="lmsbox">
  <tr>
    <td class="lms-ui-box-cell">
      <i>ikonka11</i><span class="bold">etya1 11</span>
    </td>
  </tr>
  <tr>
    <td class="lms-ui-box-cell">
      <i>ik2</i><span class="bold">etykietkaaaa2</span>      
    </td>
  </tr>
</table>

@chilek
Copy link
Owner

chilek commented Dec 17, 2018

Jaki masz tam login?

@chilek
Copy link
Owner

chilek commented Dec 17, 2018

Prawdopodobnie nie dołączyłeś do współpracy w moim jsfiddle, bo nie mam Ciebie na liście współpracowników.

@chilek
Copy link
Owner

chilek commented Dec 17, 2018

Coś takiego?
https://jsfiddle.net/chilek/xryg5kas/35/

@interduo
Copy link
Collaborator Author

Coś takiego?
https://jsfiddle.net/chilek/xryg5kas/35/

Tak. Tak jest git. Spróbuję to jutro wrzucić w kilka klas i potestować.

@interduo
Copy link
Collaborator Author

interduo commented Dec 17, 2018

Wiesz co? Próbowałem wrzucić to do schemat z jsfiddle.net do kodu rtticketinfobox.html (011a208).

Czy ten span z boldem jest na prawdę potrzebny?

@interduo
Copy link
Collaborator Author

interduo commented Dec 17, 2018

Oj wydaje mi się, że to powinno być inaczej zrobione.
W templatach HTML używamy różnych znaczników <div><span><p> w których jest <i ....></i>. Czy jest sens dla każdego definiować oddzielną klasę CSS? Wydaje mi się że pomysł z klasą lms-ui-icon-content wcale nie był taki zły.

@chilek
Copy link
Owner

chilek commented Dec 18, 2018

To tylko przykład, ale jak byś chciał robić bardziej docelowo to spójrz na parę szablonów zrobionych w oparciu o div:flexbox (oparte o funkcje i bloki smarty).

@interduo
Copy link
Collaborator Author

Ten PR pozwolił wypracować sposób kodowania przycisków i ikon. Zamykam bo jest za wiele konfliktów zmiany trzeba wprowadzić jeszcze raz.

@interduo interduo deleted the 663 branch May 13, 2020 07:46
chilek added a commit that referenced this pull request Aug 25, 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.

2 participants