# 10 UI players ranking for game mode #78
Conversation
Add component witch takes an top scores array and renders its content
tomaszdworniczak
requested review from
DomiZet,
MateuszNaKodach,
PawelSzambelan,
PiotrRynio and
annalamperska
test/components/modeRanking.spec.js
Outdated
| renderComponent(ModeRanking(emptyRanking)); | ||
|
|
||
| expect(screen.getByText('Leaderboard is empty...')).toBeInTheDocument(); | ||
| expect(screen.queryByTestId('playerRowNum1')).toBeFalsy(); |
There was a problem hiding this comment.
Dobrze napisane testy! :)
toBeFalsy ma kilka możliwośći, a zdaje mi się, żę bardziej czytelnie jest dać po prostu, że go nie ma:
expect(screen.queryByTestId('playerRowNum1')).not.toBeInTheDocument()
There was a problem hiding this comment.
racja, zamienię na notToBeInTheDocument() :P
MateuszNaKodach
left a comment
MateuszNaKodach
left a comment
There was a problem hiding this comment.
@tomdworniczak kurde, no co Ty zrobiłeś!? Nie mam za bardzo, do czego się przyczepić :) Świetnie!
Może z SCSSem by się coś znalazło, ale jeśli tak to nie będę wyręczał naszego CSSMastera @PiotrWr.
Nawet commity z oznaczeniem taska :) (pamiętaj o squash przy mergu).
src/app/components/ModeRanking.js
Outdated
| } | ||
|
|
||
| if (typeof topScores !== 'undefined' && topScores.length > 0) { | ||
| for (let i = 0; i < topScores.length; i++) { |
There was a problem hiding this comment.
Nie musicie ograniczac się w komponencie do jednej funkcji :) Dobrze byłoby dodać do osobnych np.
PlayerRow - mysle, ze to zwiększy czytelność. export zostawiacie tylko przy ModeRanking tak jak jest.
There was a problem hiding this comment.
mówisz, żeby pętle tworzące dane części wyodrębnić do osobnych funkcji?
There was a problem hiding this comment.
Może być cała pętla, ale bardziej chodziło mi o obiekt (komponent) w jej ciele.
src/app/components/ModeRanking.js
Outdated
| titleRow.appendChild(headerElement); | ||
| } | ||
|
|
||
| if (typeof topScores !== 'undefined' && topScores.length > 0) { |
There was a problem hiding this comment.
Tutaj wystarczy:
if (topScores !== undefined && topScores.length > 0)
Albo w ogóle :
if (!topScores && topScores.length > 0)
There was a problem hiding this comment.
A najlepiej to nazwać też co to dla nasz znaczy. Czyli:
const rankingIsEmpty = !topScores && topScores.length > 0
if(rankingIsEmpty){
/...cos tam tutaj..//
}
a jak się pomęczysz i wydorebnisz komponenty o których piszę niżej to może się uda coś w stylu nawet:
rankingContainer = rankingIsEmpty ? EmptyRanking() : RankingWithScores()
oczywiscie jakies tam argumenty do tych funkcji.
There was a problem hiding this comment.
hmmm fajne rozwiązanie! :-)
src/app/components/ModeRanking.js
Outdated
| rankingContainer.appendChild(playerRow); | ||
| } | ||
| } else { | ||
| const emptyRankingElem = document.createElement('p'); |
There was a problem hiding this comment.
Dobrze tak samo wyciągnąć takie dwa komponenty
EmptyRanking()
i np. RankingWithScores()
wtedy ten if będzie czytelniejszy.
|
|
||
| const headerElementTitles = ['Place', 'Player', 'Answered']; | ||
|
|
||
| for (let i = 0; i < headerElementTitles.length; i++) { |
There was a problem hiding this comment.
Tak samo, dobrze dodać funkcję, która będzie jakimś RankingHeader() i tam wszystko w środku sobie przygotować :)
Wtedy całość ModeRanking będzie bardziej czytelna.
Jest Ok ;) |
Zostawiłem trochę bałagan w App.js, ale to tylko, dlatego żebyście mogli zobaczyć jak ten komponent wygląda. Po review zakomentuję kod, który będzie odpowiedzialny za renderowanie rankingu aż do czasu 'sprzątania' :P