feat: 🍰 Hero image height on post page is now set without having to wait for… #3583

DriesCruyskens · 2020-05-28T13:50:53Z

… the image to load. Vue gives the image aspect ratio to css as a css variable, css then sets the correct height in fuction of the width.

🍰 Pullrequest

The implementation uses CSS to determine the height of the Card image and thus only affects the Post page, whereas a solution using Javascript to explicitly setting the height would need communication between the Post page and the Card component for the aspect ratio to be propagated. I think this would needlessly clutter the card component since the issue is related to the Post component only.

The solution might be seen as a 'CSS hack' to a few people but I think it is the most concise solution for the problem and is mentioned by W3 https://www.w3schools.com/howto/howto_css_aspect_ratio.asp.

Here is a GIF demonstrating the problem, before my pull request:

This is after the PR:

Issues

fixes 🚀 [Feature] Set The Teaser Image To The Appropriate Size Ratio From The Beginning #3013

Todo

None

Test

Solution works using a Neo4j cloud sandbox to simulate real loading times. Using the docker compose setup gives near instant loading times making it difficult too see the fix in action.

Front-end ✔️
Lint ✔️
Back-end ❔
End-to-end ❔

TLDR: all test should succeed, but don't on a windows machine. Someone else or CD/CI pipeline should verify.

Both end-to-end and back-end tests don't even work on a clean master git clone while the app works when developing. Since some things have to be done through WSL in order to work (like yarn test .\pages\post\_id\_slug\index.spec.js) I think this might have to do with me developing on a windows machine (windows 10 Pro 1909). This pull request/fix is pretty minimal so it shouldn't break anything end-to-end or back-end wise. If anyone with a working test suite can verify...

This is the error for end-to-end tests:

 Running:  administration\PinPost.feature                                                 (1 of 19)


  Pin a post
    √ Pinned post always appears on the top of the newsfeed (23921ms)
    √ Ordinary users cannot pin a post (15954ms)
    1) "before each" hook for "Admins are allowed to pin a post"


  2 passing (1m)
  1 failing

  1) Pin a post "before each" hook for "Admins are allowed to pin a post":
     TypeError: Cannot read property 'isAttached' of undefined

Because this error occurred during a 'before each' hook we are skipping all of the remaining tests.
      at C:\Users\dries\AppData\Local\Cypress\Cache\4.2.0\Cypress\resources\app\packages\server\lib\browsers\firefox-util.js:98:20
      at tryCatcher (C:\Users\dries\AppData\Local\Cypress\Cache\4.2.0\Cypress\resources\app\packages\server\node_modules\bluebird\js\release\util.js:16:23)
      at C:\Users\dries\AppData\Local\Cypress\Cache\4.2.0\Cypress\resources\app\packages\server\node_modules\bluebird\js\release\method.js:15:34
      at C:\Users\dries\AppData\Local\Cypress\Cache\4.2.0\Cypress\resources\app\packages\server\lib\browsers\firefox-util.js:227:40
      at tryCatcher (C:\Users\dries\AppData\Local\Cypress\Cache\4.2.0\Cypress\resources\app\packages\server\node_modules\bluebird\js\release\util.js:16:23)
      at Promise._settlePromiseFromHandler (C:\Users\dries\AppData\Local\Cypress\Cache\4.2.0\Cypress\resources\app\packages\server\node_modules\bluebird\js\release\promise.js:547:31)
      at Promise._settlePromise (C:\Users\dries\AppData\Local\Cypress\Cache\4.2.0\Cypress\resources\app\packages\server\node_modules\bluebird\js\release\promise.js:604:18)
      at Promise._settlePromise0 (C:\Users\dries\AppData\Local\Cypress\Cache\4.2.0\Cypress\resources\app\packages\server\node_modules\bluebird\js\release\promise.js:649:10)
      at Promise._settlePromises (C:\Users\dries\AppData\Local\Cypress\Cache\4.2.0\Cypress\resources\app\packages\server\node_modules\bluebird\js\release\promise.js:729:18)
      at _drainQueueStep (C:\Users\dries\AppData\Local\Cypress\Cache\4.2.0\Cypress\resources\app\packages\server\node_modules\bluebird\js\release\async.js:93:12)
      at _drainQueue (C:\Users\dries\AppData\Local\Cypress\Cache\4.2.0\Cypress\resources\app\packages\server\node_modules\bluebird\js\release\async.js:86:9)
      at Async._drainQueues (C:\Users\dries\AppData\Local\Cypress\Cache\4.2.0\Cypress\resources\app\packages\server\node_modules\bluebird\js\release\async.js:102:5)
      at Immediate.Async.drainQueues [as _onImmediate] (C:\Users\dries\AppData\Local\Cypress\Cache\4.2.0\Cypress\resources\app\packages\server\node_modules\bluebird\js\release\async.js:15:14)
      at processImmediate (internal/timers.js:439:21)

And these are the failed back-end tests:

Summary of all failing tests
 FAIL  src/schema/resolvers/reports.spec.js (126.348s)
  ● file a report on a resource › query for reported resource › unauthenticated › throws authorization error

    Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.Error: Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.

      at mapper (node_modules/jest-jasmine2/build/queueRunner.js:27:45)

 FAIL  src/middleware/notifications/notificationsMiddleware.spec.js (61.567s)
  ● notifications › authenticated › given another user › mentions me in a post › updates the post and 
mentions me again › if the notification was marked as read earlier › but the next mention happens after the notification was marked as read › sets the `read` attribute to false again

    : Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.Error:
      378 | 
      379 |             describe('but the next mention happens after the notification was marked as read', () => {
    > 380 |               it('sets the `read` attribute to false again', async () => {
          |               ^
      381 |                 await createPostAction()
      382 |                 await markAsReadAction()
      383 |                 const {

      at new Spec (node_modules/jest-jasmine2/build/jasmine/Spec.js:116:22)
      at Suite.<anonymous> (src/middleware/notifications/notificationsMiddleware.spec.js:380:15)
      at Suite.<anonymous> (src/middleware/notifications/notificationsMiddleware.spec.js:379:13)
      at Suite.<anonymous> (src/middleware/notifications/notificationsMiddleware.spec.js:367:11)
      at Suite.<anonymous> (src/middleware/notifications/notificationsMiddleware.spec.js:307:9)
      at Suite.<anonymous> (src/middleware/notifications/notificationsMiddleware.spec.js:253:7)
      at Suite.<anonymous> (src/middleware/notifications/notificationsMiddleware.spec.js:108:5)
      at Suite.<anonymous> (src/middleware/notifications/notificationsMiddleware.spec.js:103:3)
      at Object.<anonymous> (src/middleware/notifications/notificationsMiddleware.spec.js:81:1)

  ● notifications › authenticated › given another user › mentions me in a post › updates the post and 
mentions me again › if the notification was marked as read earlier › but the next mention happens after the notification was marked as read › does not update the `createdAt` attribute

    : Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.Error:
      400 |               })
      401 | 
    > 402 |               it('does not update the `createdAt` attribute', async () => {
          |               ^
      403 |                 await createPostAction()
      404 |                 await markAsReadAction()
      405 |                 const {

      at new Spec (node_modules/jest-jasmine2/build/jasmine/Spec.js:116:22)
      at Suite.<anonymous> (src/middleware/notifications/notificationsMiddleware.spec.js:402:15)
      at Suite.<anonymous> (src/middleware/notifications/notificationsMiddleware.spec.js:379:13)
      at Suite.<anonymous> (src/middleware/notifications/notificationsMiddleware.spec.js:367:11)
      at Suite.<anonymous> (src/middleware/notifications/notificationsMiddleware.spec.js:307:9)
      at Suite.<anonymous> (src/middleware/notifications/notificationsMiddleware.spec.js:253:7)
      at Suite.<anonymous> (src/middleware/notifications/notificationsMiddleware.spec.js:108:5)
      at Suite.<anonymous> (src/middleware/notifications/notificationsMiddleware.spec.js:103:3)
      at Object.<anonymous> (src/middleware/notifications/notificationsMiddleware.spec.js:81:1)


Test Suites: 2 failed, 38 passed, 40 total
Tests:       3 failed, 1 skipped, 1 todo, 455 passed, 460 total
Snapshots:   0 total
Time:        1342.549s, estimated 1356s

… the image to load. Vue gives the image aspect ratio to css as a css variable, css then sets the correct height in fuction of the width.

cypress · 2020-05-28T13:58:18Z

Test summary

66 • 0 • 0 • 0

Run details


Project	Human-Connection
Status	Passed
Commit	`e33a190`
Started	Jun 6, 2020 8:25 AM
Ended	Jun 6, 2020 8:44 AM
Duration	19:37 💡
OS	Linux Ubuntu Linux - 16.04
Browser	Firefox 68

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

cypress · 2020-05-28T14:27:54Z

Test summary

66 • 0 • 0 • 0

Run details


Project	Human-Connection
Status	Passed
Commit	`b3fd3f1` ℹ️
Started	Jun 6, 2020 8:25 AM
Ended	Jun 6, 2020 8:44 AM
Duration	19:12 💡
OS	Linux Ubuntu Linux - 16.04
Browser	Firefox 68

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

DriesCruyskens · 2020-05-28T14:50:52Z

@Tirokk Any idea as to why the back-end tests fail? It seems to be different errors then my local ones. I'll try local tests on an Ubuntu VM in the meantime to rule out the windows quirkiness.

DriesCruyskens · 2020-05-29T12:11:36Z

@Tirokk Any idea as to why the back-end tests fail? It seems to be different errors then my local ones. I'll try local tests on an Ubuntu VM in the meantime to rule out the windows quirkiness.

n.v.m, seems they succeeded on a subsequent run.

Tirokk · 2020-05-29T12:46:27Z

I just restarted the failing build … 🤗
The builds seem sometimes to be a bit unstable. Especially the Cypress tests.

If they fail we can just restart them. There are some guesses of @rbeer why it is this way, but no time at the moment to change this …

Tirokk

Hey @DriesCruyskens !
Wonderful you did your first PR. 🍾💫

I’ve tried this out with my images and created a post.
Unfortunately it didn’t worked with both images.
But in the production it worked correctly …

Here you see what I got …

Original image:

Created a post and dragged the image in the teaser area …

Clipping, correct so far:

Post before saved, correct so far:

Post after save, wrong:

Post in newsfeed, correct so far:

I don’t now what the reason is till now and have no time to find out before the weekend.

Tirokk · 2020-05-29T14:43:52Z

Same with an other image as well …

Original image:

Created a post and dragged the image in the teaser area …

Clipping, correct so far:

Post before saved, correct so far:

Post after save, wrong:

Post in newsfeed, correct so far:

I’ll be back after our holidays till Monday on Wednesday, because I have Tuesdays off.

DriesCruyskens · 2020-05-29T15:30:39Z

That's on me then for not testing it rigorously enough, I will look into it.

DriesCruyskens · 2020-05-29T17:06:20Z

I fixed the issue. I had to inverse the aspect ratio since I calculate the height based on the width and aspect ratio is width over height. As you can see in your screenshots, the size of the image container is like the image was rotated sideways. I also updated the PR request with updated GIFs of the fix in action.

Here are the screenshots after the fix using your aptly chosen images:

Requested change done

Tirokk

Hey @DriesCruyskens ,
this is wonderful work of yours !!! 🚀💫

Your solution works great for me and as well if I jump to comments I got notified of!
Love it! 😍

I'm glad that you devoted your time to our project!

Tirokk · 2020-06-03T09:34:57Z

Just in case somebody has time and because I'm not the CSS specialist, what are other peoples opinions?
@mattwr18 @Mogge @roschaefer @alina-beck

Otherwise I will merge it like it is …

…te-size-ratio-from-the-beginning

- My conflict resolved on GitHub missed a bracket.

Hero image height on post page is now set withouth having to wait for…

abdc807

… the image to load. Vue gives the image aspect ratio to css as a css variable, css then sets the correct height in fuction of the width.

DriesCruyskens changed the title ~~feat: Hero image height on post page is now set withouth having to wait for…~~ feat: [WIP] Hero image height on post page is now set without having to wait for… May 28, 2020

DriesCruyskens changed the title ~~feat: [WIP] Hero image height on post page is now set without having to wait for…~~ feat: Hero image height on post page is now set without having to wait for… May 29, 2020

Tirokk changed the title ~~feat: Hero image height on post page is now set without having to wait for…~~ feat: 🍰 Hero image height on post page is now set without having to wait for… May 29, 2020

Tirokk previously requested changes May 29, 2020

View reviewed changes

Inversed the aspect ratio to fix bug.

4ab7972

Tirokk approved these changes Jun 3, 2020

View reviewed changes

Tirokk requested review from Mogge, mattwr18, alina-beck and roschaefer June 3, 2020 09:32

Tirokk added 2 commits June 6, 2020 09:50

Merge branch 'master' into 3013-set-the-teaser-image-to-the-appropria…

cf3f9fa

…te-size-ratio-from-the-beginning

Fix syntax

e33a190

- My conflict resolved on GitHub missed a bracket.

Tirokk merged commit c113fde into master Jun 6, 2020

Tirokk deleted the 3013-set-the-teaser-image-to-the-appropriate-size-ratio-from-the-beginning branch June 6, 2020 09:52

Tirokk assigned DriesCruyskens Jun 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 🍰 Hero image height on post page is now set without having to wait for… #3583

feat: 🍰 Hero image height on post page is now set without having to wait for… #3583

DriesCruyskens commented May 28, 2020 •

edited

cypress bot commented May 28, 2020 •

edited

cypress bot commented May 28, 2020 •

edited

DriesCruyskens commented May 28, 2020

DriesCruyskens commented May 29, 2020

Tirokk commented May 29, 2020 •

edited

Tirokk left a comment •

edited

Tirokk commented May 29, 2020

DriesCruyskens commented May 29, 2020

DriesCruyskens commented May 29, 2020

Tirokk left a comment •

edited

Tirokk commented Jun 3, 2020 •

edited

feat: 🍰 Hero image height on post page is now set without having to wait for… #3583

feat: 🍰 Hero image height on post page is now set without having to wait for… #3583

Conversation

DriesCruyskens commented May 28, 2020 • edited

🍰 Pullrequest

Issues

Todo

Test

cypress bot commented May 28, 2020 • edited

Test summary

Run details

cypress bot commented May 28, 2020 • edited

Test summary

Run details

DriesCruyskens commented May 28, 2020

DriesCruyskens commented May 29, 2020

Tirokk commented May 29, 2020 • edited

Tirokk left a comment • edited

Choose a reason for hiding this comment

Tirokk commented May 29, 2020

DriesCruyskens commented May 29, 2020

DriesCruyskens commented May 29, 2020

Tirokk left a comment • edited

Choose a reason for hiding this comment

Tirokk commented Jun 3, 2020 • edited

DriesCruyskens commented May 28, 2020 •

edited

cypress bot commented May 28, 2020 •

edited

cypress bot commented May 28, 2020 •

edited

Tirokk commented May 29, 2020 •

edited

Tirokk left a comment •

edited

Tirokk left a comment •

edited

Tirokk commented Jun 3, 2020 •

edited