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

fix(files): rework store for files modal #2745

Merged
merged 3 commits into from
Apr 11, 2022
Merged

fix(files): rework store for files modal #2745

merged 3 commits into from
Apr 11, 2022

Conversation

josephmcg
Copy link
Contributor

@josephmcg josephmcg commented Apr 6, 2022

What this PR does πŸ“–

  • if you refresh and then open file, it needs to fetch the blob from textile for download. the way state is handled for this modal crashes. PR fixes this crash
  • less overhead in store, only keep track of name rather than full Fil

Which issue(s) this PR fixes πŸ”¨

Special notes for reviewers πŸ—’οΈ

Additional comments 🎀

@github-actions github-actions bot added the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Apr 6, 2022
@netlify
Copy link

netlify bot commented Apr 6, 2022

βœ… Yeeeehaw, deploy preview is ready!

Name Link
πŸ”¨ Latest commit b06b55b
πŸ” Latest deploy log https://app.netlify.com/sites/adoring-edison-dbcef8/deploys/624d2bc0341f270008cc3a5a
😎 Deploy Preview https://deploy-preview-2745--adoring-edison-dbcef8.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Apr 6, 2022

βœ… Yeeeehaw, deploy preview is ready!

Name Link
πŸ”¨ Latest commit da21438
πŸ” Latest deploy log https://app.netlify.com/sites/adoring-edison-dbcef8/deploys/6253e1ebbd833d00085979f4
😎 Deploy Preview https://deploy-preview-2745--adoring-edison-dbcef8.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions bot added the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Apr 6, 2022
@josephmcg josephmcg removed the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Apr 7, 2022
@phillsatellite phillsatellite added the QA tested One QA Person has tested - Needs QA Lead review still label Apr 7, 2022
@molimauro molimauro removed the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Apr 8, 2022
@stavares843 stavares843 force-pushed the filesFix branch 2 times, most recently from 1e6635e to fe73340 Compare April 8, 2022 18:20
Copy link
Member

@stavares843 stavares843 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sending a .heic

Captura de ecrã 2022-04-08, às 19 32 18

dev

vs this branch
Captura de ecrã 2022-04-08, às 19 32 31

The same happens with a .avif

dev

Captura de ecrã 2022-04-08, às 19 33 06

this branch

Captura de ecrã 2022-04-08, às 19 33 49

@stavares843 stavares843 added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed QA tested One QA Person has tested - Needs QA Lead review still labels Apr 8, 2022
@josephmcg
Copy link
Contributor Author

This does not impact chat messages

@josephmcg josephmcg added Ready for QA Ready for QA team to test, Devs approved. and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Apr 11, 2022
@stavares843
Copy link
Member

if doesn't impact chat messages, why the difference in behavior above? @josephmcg

@stavares843 stavares843 added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed Ready for QA Ready for QA team to test, Devs approved. labels Apr 11, 2022
@josephmcg
Copy link
Contributor Author

I would suggest getting the latest rebased branch and running it locally, they're not my changes so I have no idea why chat is acting like that

@josephmcg
Copy link
Contributor Author

seems like our netlify pipeline takes commits from everywhere? idk, i never use it besides mobile testing. that would support the 'someone else's code' theory.

https://app.netlify.com/sites/adoring-edison-dbcef8/deploys

@josephmcg josephmcg added Ready for QA Ready for QA team to test, Devs approved. and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Apr 11, 2022
@stavares843
Copy link
Member

the netlify commits are the ones on this branch, it takes this branch code changes and then deploys to netlify

but I can test locally as well

@stavares843
Copy link
Member

checked locally and now is fine and has the same behavior in both dev and this branch

thanks for checking πŸ”¨

@stavares843 stavares843 merged commit 58f0b22 into dev Apr 11, 2022
@stavares843 stavares843 deleted the filesFix branch April 11, 2022 12:13
@github-actions github-actions bot removed the Ready for QA Ready for QA team to test, Devs approved. label Apr 11, 2022
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.

None yet

5 participants