okfn / opendataeditor

The Open Data Editor (ODE) is a no-code application to explore, validate and publish data in a simple way. Forever free and open source project powered by the Frictionless Framework.
http://opendataeditor.okfn.org
MIT License
183 stars 23 forks source link

[WiP] Selecting multiple files for deleting on left menu #368

Closed guergana closed 5 months ago

guergana commented 6 months ago
cloudflare-workers-and-pages[bot] commented 6 months ago

Deploying opendataeditor with  Cloudflare Pages  Cloudflare Pages

Latest commit: f0a32f0
Status: ✅  Deploy successful!
Preview URL: https://75fad6b3.opendataeditor.pages.dev
Branch Preview URL: https://341-selecting-multiple-files.opendataeditor.pages.dev

View logs

guergana commented 5 months ago

@roll I need your help with this PR. I have some issues I can't figure out on my own, besides the fact that I am changing parts of the application in this ticket that I am not sure which side effects they could have..

I don't understand why here: https://github.com/okfn/opendataeditor/blob/main/client/components/Application/Content.tsx#L28 ? the FileContent is updated and fired even if the client and the record stay the same. If you test, when you select one File in the left menu it loads correctly, but when you select a second file, in theory, the FileContent component should stay the same, but it updates and returns a blank screen. Could you help me debug and test since you are the most familiar with all parts of the application? 🙏

roll commented 5 months ago

@guergana @pdelboca As we don't yet have tests for this store, and the complexity is already very high, I think the safest approach here would be to introduce a completely new state property like secondaryPaths with completely separated logic that only works for selection and deleting (the user story requirement), for example:

I think this kind of implementation at least won't break any existent functionality. WDYT?

guergana commented 5 months ago

@guergana @pdelboca As we don't yet have tests for this store, and the complexity is already very high, I think the safest approach here would be to introduce a completely new state property like secondaryPaths with completely separated logic that only works for selection and deleting (the user story requirement), for example:

* on file click we set `path='file1'`

* on ctrl-click on other file we set `secondaryPaths=['file2']`

* [handling selecting / deselecting / other logic ]

* on "Delete" click we just delete `path` and all the `secondaryPaths`

I think this kind of implementation at least won't break any existent functionality. WDYT?

@roll I thought about doing something similar like paths when it was a multiselect and only path for single select, but then also thought this is not a very elegant solution... this is why i went for changing the whole store, but yes, as you point out, this is a very risky approach since we don't have tests throught the application and i might have broken something. I agree your approach is a good solution for now.

guergana commented 5 months ago

@guergana @pdelboca As we don't yet have tests for this store, and the complexity is already very high, I think the safest approach here would be to introduce a completely new state property like secondaryPaths with completely separated logic that only works for selection and deleting (the user story requirement), for example:

* on file click we set `path='file1'`

* on ctrl-click on other file we set `secondaryPaths=['file2']`

* [handling selecting / deselecting / other logic ]

* on "Delete" click we just delete `path` and all the `secondaryPaths`

I think this kind of implementation at least won't break any existent functionality. WDYT?

@roll I am trying to implement your proposed approach but I think it will also complicate the logic. I can't find a way to implement this in a simple way. Would you mind taking over this task and implementing your proposed approach?

guergana commented 5 months ago

Finally, @pdelboca s proposed approach worked better and I have implemented it in a different PR. I will close this PR in favor of https://github.com/okfn/opendataeditor/pull/426