Ref T1500
Details
Details
- Reviewers
arkadiusz-wieczorek kuba-orlik - Group Reviewers
Unknown Object (Project) - Maniphest Tasks
- Unknown Object (Maniphest Task)
- Commits
- rSEALPAGE3e73509457e3: Dodanie obsługi .webp do komponentu responsive-image
Diff Detail
Diff Detail
- Repository
- rSEALPAGE Sealpage
- Branch
- webp-resp-img
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 1755 Build 1755: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| components/responsive-image/responsive-image.html.js | ||
|---|---|---|
| 20 | Trzeba jeszcze komponent dostosować do nowego API (klasa Component - zob. np. komponent [Markdown](https://hub.sealcode.org/diffusion/SEALPAGE/browse/master/components/markdown/markdown.html.js) | |
Comment Actions
Accept, z jedną zmianą
| components/responsive-image/responsive-image.html.js | ||
|---|---|---|
| 105–122 | zbyt rozległy zapis. Wystarczy: renderFn: responsive_image_creator, | |
Comment Actions
W tym momencie nie można wylądować ze względu na występujący error
TypeError: Error.captureStackTrace is not a function
Comment Actions
Podejrzewam, że przyczyną tych błędów jest fakt, że na front idzie kod z depsami przeznaczonymi tylko na backend (np. ta libka do obróbki zdjęć). Mam proponowane rozwiązanie:
Comment Actions
- merge
- initial responsive-image refactor for webp support
- removed unnecessary files and fixed tests for the new api
Comment Actions
Proponuję w dokumentacji (i w kodzie?) zmienić nazwę Plugins na ExternalComponents. Taka nazwa będzie imho lepiej odzwierciedlała stan faktyczny. "Plugin" będzie lepiej zarezerwować na przyszłość, kiedy będą jakieś paczki z dużą ilością powiązanych funkcjonalności, np. plugin do robienia blogów, itp. Wiem, że trochę późno to zgłaszam, ale pomoże nam to zapobiec problemom na przyszłość
| README.md | ||
|---|---|---|
| 87–88 ↗ | (On Diff #2373) | A czy tu nie trzeba najpierw zaimportować Sealpage'a? Żeby Component nie było undefined? |
| 128 ↗ | (On Diff #2373) | (...) - an array of plugins |
| 129 ↗ | (On Diff #2373) | into *the* array |
| 160 ↗ | (On Diff #2373) | what do display => what is being displayed |
| 170–177 ↗ | (On Diff #2373) | tutaj mamy trochę kolizję na słowie control, co utrudnia czytelnosć. Proponuję zmienić ten używany w przykładach komponent na Greet, który we wcześniejszym przykładzie pisze tylko Hello, a w późniejszym dostaje prop-a name i pisze Hello, ${name} |
| components/responsive-image/responsive-image.html.js | ||
| 84 | zamiast robić tu warunek specjalny dla webp proponuję w argsach podawać po prostu config osobno dla każdego formatu: {
webp: {..},
png: {...}
} | |
| 84–85 | { i return są tutaj niepotrzebne, można zostawić gołe arrow function; ext => resolutions.map //... | |
| 88 | jeżeli źródłowy plik jest w png, to nie konwertujmy go do jpg, bo to może powodować niechciane artefakty. Proponuję abyśmy zawsze brali webp + format źródłowego zdjęcia. Np. webp+png, webp+jpeg. | |
| 92 | do depsów trzeba też dodać config - wybraną dla aktualnego pliku rozdzielczość, format, jakość itp. Aby ich zmiana powodowała przerenderowanie | |
Comment Actions
wcześniej npm test mi przechodziło, ale teraz rzuca:
0 passing (220ms)
2 failing
1) responsive-image-creator
Should create images with given resolutions:
Error: ENOENT: no such file or directory, access '/home/kuba/projects/midline/sealpage/components/responsive-image/test/images/test-img-200.cdb2366880b69ed5764e43ad0b19c728.webp'
at Object.accessSync (fs.js:197:3)
at Context.<anonymous> (components/responsive-image/test/responsive-image.test.js:41:8)
2) responsive-image-creator
Should resolve a valid responsive html picture tag:
AssertionError [ERR_ASSERTION]: '<picture><sourcesrcset="images/test-img-200.07814c16e5bde1564ae925bf47cffe5e.webp200w,images/test-img-300.07814c16e5bde1564ae925bf47cffe5e.webp300w"sizes="100px"alt="alt"/><sourcesrcset="images/test-img-200.07814c16e5bde1564ae925bf47cffe5e.jpg200w,images/test-img-300.07814c16e5bde1564ae925bf47cffe5e.jpg300w"sizes="100px"alt="alt"/><imgsrc="images/test-img-300.07814c16e5bde1564ae925bf47cffe5e.jpg"alt="alt"/></picture>' == '<picture><sourcesrcset="images/test-img-200.cdb2366880b69ed5764e43ad0b19c728.webp200w,images/test-img-300.cdb2366880b69ed5764e43ad0b19c728.webp300w"sizes="100px"alt="alt"/><sourcesrcset="images/test-img-200.cdb2366880b69ed5764e43ad0b19c728.jpg200w,images/test-img-300.cdb2366880b69ed5764e43ad0b19c728.jpg300w"sizes="100px"alt="alt"/><imgsrc="images/test-img-300.cdb2366880b69ed5764e43ad0b19c728.jpg"alt="alt"/></picture>'
+ expected - actual
-<picture><sourcesrcset="images/test-img-200.07814c16e5bde1564ae925bf47cffe5e.webp200w,images/test-img-300.07814c16e5bde1564ae925bf47cffe5e.webp300w"sizes="100px"alt="alt"/><sourcesrcset="images/test-img-200.07814c16e5bde1564ae925bf47cffe5e.jpg200w,images/test-img-300.07814c16e5bde1564ae925bf47cffe5e.jpg300w"sizes="100px"alt="alt"/><imgsrc="images/test-img-300.07814c16e5bde1564ae925bf47cffe5e.jpg"alt="alt"/></picture>
+<picture><sourcesrcset="images/test-img-200.cdb2366880b69ed5764e43ad0b19c728.webp200w,images/test-img-300.cdb2366880b69ed5764e43ad0b19c728.webp300w"sizes="100px"alt="alt"/><sourcesrcset="images/test-img-200.cdb2366880b69ed5764e43ad0b19c728.jpg200w,images/test-img-300.cdb2366880b69ed5764e43ad0b19c728.jpg300w"sizes="100px"alt="alt"/><imgsrc="images/test-img-300.cdb2366880b69ed5764e43ad0b19c728.jpg"alt="alt"/></picture>
at Context.<anonymous> (components/responsive-image/test/responsive-image.test.js:79:10)| README.md | ||
|---|---|---|
| 179 ↗ | (On Diff #2386) | Tu chyba lepiej dać 'text' :) |
| components/responsive-image/responsive-image.html.js | ||
| 76 | image_path można stąd wyrzucić. Jeżeli file_hash się nie zmieniło, to nie ma potrzeby re-renderować tylko jak sie zmieni nazwa pliku | |
| 80 | alt też można wyrzucić z tej listy. nie ma wpływu na plik wynikowy | |
| 81 | tutaj najlepiej byłoby dać tylko te optionsy, które dotyczą aktualnie obrabianego typu pliku, np. tylko webp albo tylko jpg | |
| components/responsive-image/test/responsive-image.test.js | ||
|---|---|---|
| 24–29 | Myślę, że ten test zaczyna być powoli kopią implementacji samego komponentu. Jak już to chcemy testować, to proponuję zamiast sprawdzania wartości hasza w nazwie pliku sprawdzać coś bardziej wysokopoziomowego, a mianowicie: czy hasz w pliku się zmienia, jeżeli plik źródłowy się zmienia? Nie musimy wtedy przywiązywać się do konkretnej wartości hasha ani nawet funkcji haszującej. Wywołajmy komponent raz, i sprawdźmy, czy daje prawidłowo sformowany obrazek (w tym celu najlepiej sprawdzi się jakaś libka do parsowania fragmentów html-a). Następnie wywołujemy komponent drugi raz i sprawdzamy, że daje dokładnie taki sam wynik jak poprzednim razem. Następnie Wywołujemy trzeci raz, ale zmieniamy tylko obrazek źródłowy - i powinno dać inny rezultat | |
| components/responsive-image/test/responsive-image.test.js | ||
|---|---|---|
| 40–43 | Spromisifikumy to dla czytelności https://nodejs.org/api/util.html#util_util_promisify_original | |
| 68–72 | nie musimy trzymać srcset i pattern w osobnej stałej, zmniejsza to czytelność. Proponuję po prostu: assert( /images\/test-img-\d{3}\.[a-z0-9]{32}\.(webp|jpg)\s[0-9]{3}w/.test( child.attribs.srcset ) ); | |
| 107–109 | Te asserty są w callbacku, więc wydaje się, że nie zostaną wychwycone przez Mocha. Z jakiegoś powodu jednak są wychwytane, ale nie wiem jakim cudem. Niemniej jednak polecam spromisifikować readdir | |