search

aabounegm / cast

A podcast listening progressive web app with all-around automated quality assurance
https://cast-iu.pages.dev
MIT License
22 stars 2 forks source link

No layout shifts #215

Closed illright closed 2 years ago

illright commented 2 years ago

Closes #212 and closes #216. But I can't tell if the performance is actually worse now, Lighthouse is being kinda weird. The app certainly feels better now, so idk

cloudflare-workers-and-pages[bot] commented 2 years ago

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2f3b98c
Status: ✅  Deploy successful!
Preview URL: https://e38166ce.cast-iu.pages.dev

View logs

illright commented 2 years ago

Or maybe it feels worse

codecov-commenter commented 2 years ago

Codecov Report

Merging #215 (2f3b98c) into main (4da59a1) will not change coverage. The diff coverage is 27.27%.

@@           Coverage Diff           @@
##             main     #215   +/-   ##
=======================================
  Coverage   62.96%   62.96%           
=======================================
  Files          69       69           
  Lines         613      613           
  Branches      165      165           
=======================================
  Hits          386      386           
  Misses        219      219           
  Partials        8        8
Impacted Files Coverage Δ
src/lib/app/lib/handle-hook.ts 0.00% <0.00%> (ø)
src/lib/entities/podcast/api/requests.ts 21.42% <ø> (ø)
src/lib/pages/podcast-gallery/api/load-podcasts.ts 0.00% <0.00%> (ø)
src/lib/features/listening-history/model/store.ts 81.25% <100.00%> (ø)
...ures/read-transcript/ui/skeleton-transcript.svelte 100.00% <100.00%> (ø)
src/lib/entities/podcast/model/podcast-store.ts

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4da59a1...2f3b98c. Read the comment docs.

illright commented 2 years ago

No, it's actually better, but there's still a layout shift for when there's no listening history

illright commented 2 years ago

@aabounegm can you handle the Applitools differences?

aabounegm commented 2 years ago

I did, but it doesn't update the workflow status for the integration tests.

Leaving this link here for the future, but probably even this wouldn't update the original workflow status: https://applitools.com/blog/link-github-actions/ Perhaps we can combine the link above with a cy.on('fail', console.error) inside the relevant test

VanishMax commented 2 years ago

@illright Why won't we just remove the "Recently listened" when there's no podcasts? Or, actually, we could keep track of listened podcasts in the cookies instead of LocalStorage, so this data is fulfilled on the backend

illright commented 2 years ago

You're right, actually, storing the history in cookies might be a great idea to optimize this

illright commented 2 years ago

@aabounegm I didn't really understand what that link you sent was for. Could you please accept the diffs again :p?

aabounegm commented 2 years ago

The link was about a dedicated workflow for checking the Applitools status. As I said earlier, approving the changes in Applitools' dashboard does not change Cypress' check status into a green checkmark. I'm also not sure if that link helps with this issue.

But also, I think the issue here makes sense. There might not be large content shifts anymore, but there is a header with empty content. I believe the behavior on the left is still desirable.

image