matrix-react-sdk icon indicating copy to clipboard operation
matrix-react-sdk copied to clipboard

Published 20 hours ago • verified publisher icon matrix-org

Implement download_file in widget driver

Open weeman1337 opened this issue 1 year ago • 8 comments

Implement the widget API download_file in the widget driver.

Needs https://github.com/matrix-org/matrix-widget-api/pull/99

Checklist

  • [x] Tests written for new code (and old code if feasible).
  • [x] New or updated public/exported symbols have accurate TSDoc documentation.
  • [x] Linter and other CI checks pass.
  • [x] Sign-off given on the changes (see CONTRIBUTING.md).

weeman1337 avatar Aug 27 '24 08:08 weeman1337

Hi! Can you first of all fix the lint and ts errors? Thanks!

florianduros avatar Aug 28 '24 07:08 florianduros

Hi! Can you first of all fix the lint and ts errors? Thanks!

The „lint“ errors are actually type check errors caused by this PR relying on https://github.com/matrix-org/matrix-widget-api/pull/99

weeman1337 avatar Aug 28 '24 08:08 weeman1337

here's described how to test everything together: https://github.com/matrix-org/matrix-widget-api/pull/99#issuecomment-2314962447

HarHarLinks avatar Aug 28 '24 11:08 HarHarLinks

The „lint“ errors are actually type check errors caused by this PR relying on https://github.com/matrix-org/matrix-widget-api/pull/99

I think you should be able to update the package.json to use your widget api pr for the review and revert the change once the widget api is merged. Than the two PR's can be reviewed simultaneously with both being checked by the linter already.

To be more specific: Change: https://github.com/matrix-org/matrix-react-sdk/blob/develop/package.json#L118 to "matrix-widget-api: "github:nordeck/matrix-widget-api#download-file" (I think)

toger5 avatar Aug 28 '24 13:08 toger5

The change in the package.json made it worse

florianduros avatar Aug 29 '24 08:08 florianduros

The change in the package.json made it worse

Hello @florianduros, could you please advise us what you need us to do to get this reviewed? Like weeman said, the lint errors are due to the wrong dependency version. How should we fix it?

HarHarLinks avatar Aug 29 '24 12:08 HarHarLinks

I'll review it in the afternoon. Why not waiting for the https://github.com/matrix-org/matrix-widget-api/pull/99 PR to be released to ask for a review? It can't be merged without it.

florianduros avatar Aug 29 '24 12:08 florianduros

Why not waiting for the matrix-org/matrix-widget-api#99 PR to be released to ask for a review? It can't be merged without it.

Code for both (and also a widget) is already done so we could test it end to end. We're asking for reviews of both PRs that target element repos. It seems to us to make sense to review both PRs at the same time.

Some info maybe not obvious from the title and OP: this is a (partial) implementation of MSC4039.

HarHarLinks avatar Aug 29 '24 12:08 HarHarLinks

I've merged the widgets api PR and released it as 1.9.0 so you'll need tov change the dependency here (or allow edits so we can).

dbkr avatar Aug 30 '24 13:08 dbkr